Add pytests for preprocessing by JMGilbert · Pull Request #67 · ClimateImpactLab/dscim

JMGilbert · 2023-01-20T21:23:00Z

No description provided.

codecov-commenter · 2023-01-20T21:29:36Z

Codecov Report

Merging #67 (e0244d8) into dscim-v0.4.0 (3d1242b) will increase coverage by 6.46%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           dscim-v0.4.0      #67      +/-   ##
================================================
+ Coverage         39.94%   46.40%   +6.46%     
================================================
  Files                17       17              
  Lines              1865     1866       +1     
================================================
+ Hits                745      866     +121     
+ Misses             1120     1000     -120

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

JMGilbert · 2023-01-23T21:33:44Z

dscim/src/dscim/preprocessing/preprocessing.py

Lines 163 to 188 in cda25bc

    
           def reformat_climate_files(): 
        
               from dscim.preprocessing.climate.reformat import ( 
        
                   convert_old_to_newformat_AR, 
        
                   stack_gases, 
        
               ) 
        
               # convert AR6 files 
        
               bd = "/shares/gcp/integration/float32/dscim_input_data/climate/AR6" 
        
               pathdt = { 
        
                   "median": f"{bd}/ar6_fair162_medianparams_control_pulse_2020-2080_10yrincrements_conc_rf_temp_lambdaeff_emissions-driven_2naturalfix_v4.0_Jan212022.nc", 
        
                   "sims": f"{bd}/ar6_fair162_control_pulse_2020-2030-2040-2050-2060-2070-2080_emis_conc_rf_temp_lambdaeff_emissions-driven_naturalfix_v4.0_Jan212022.nc", 
        
               } 
        
               newds = convert_old_to_newformat_AR( 
        
                   pathdt, 
        
                   gas="CO2_Fossil", 
        
                   var="temperature", 
        
               ) 
        
               newds.to_netcdf( 
        
                   f"{bd}/ar6_fair162_sim_and_medianparams_control_pulse_2030-2040-2050-2060-2070-2080_emis_conc_rf_temp_lambdaeff_emissions-driven_naturalfix_v4.0_Jan212022.nc" 
        
               ) 
        
               # convert RFF files 
        
               gases = {"CO2_Fossil": "Feb072022", "CH4": "Feb072022", "N2O": "Feb072022"} 
        
               stack_gases(gas_dict=gases)

should either be generalized or removed. If generalized, we just need the one more test to finish tests for dscim/preprocesing/preprocessing.py.

JMGilbert · 2023-01-23T21:36:18Z

src/dscim/preprocessing/preprocessing.py

    )
-
-
-def clip_damages(


I didn't find reference to this function anywhere except dscim-cil which only references it in this script. If this script were updated to be operational, this function would be removed entirely.

@JMGilbert can you create a new issue about removing this, then do a new PR that removes this function entirely? Then we can reference the PR in dscim-cil where your link points. Thank you!

Noting here that we decided this doesn't need an Issue. It's noted in the CHANGELOG

JMGilbert · 2023-02-16T22:10:35Z

See #71

kemccusker

This is great, @JMGilbert thank you! I know these tests are beasts. I have a couple questions and flagged where we will probably need updates due to recent other merges.

tests/test_preprocessing.py

…actLab/dscim into preprocessing_pytests

brews

I was asked to have a look at this because testing untested legacy code can be such a headache, especially when it deals with IO and files like this. This is awesome work. Great use of the tmp_path fixture for temporary files. I left a few inline comments with some pointers to help make these cleaner to read and understand when they fail.

brews · 2023-04-11T22:43:04Z

tests/test_preprocessing.py

+    xr.testing.assert_equal(ds_out_expected, ds_out_actual)
+
+
+def test_sum_AMEL(tmp_path):


Add a docstr here giving one line saying the one behavior this test is trying to check. At this stage, testing legacy code, I think it's okay if it's something “Tests the basic functionality of [insert function name here]“. But try to have each test check one behavior or have one reason to fail.

There are a couple other missing docstrs below. Ill try to call them all out.

brews · 2023-04-11T22:43:48Z

tests/test_preprocessing.py

+        ("risk_aversion", 10, 15),
+    ],
+)
+def test_reduce_damages(tmp_path, recipe, eta, batchsize):


Add a docstr here saying what you're testing.

brews · 2023-04-11T22:44:18Z

tests/test_preprocessing.py

+        ("risk_aversion", 10, "cheese", True),
+    ],
+)
+def test_ce_from_chunk(tmp_path, recipe, eta, reduction, zero):


Add docstr briefly saying what you're testing.

brews · 2023-04-11T23:04:41Z

tests/test_preprocessing.py

+    ],
+)
+def test_reduce_damages(tmp_path, recipe, eta, batchsize):
+    if recipe == "adding_up" and eta is not None:


The if/elses here hint this test is testing more than one behavior. I'd consider breaking it down into multiple test functions so that each one is more clearly checking for one behavior or has one reason to fail. So, in this case, maybe make one test function that checks that this error gets raised, and then another test function checking "the happy path" - testing the functions spits out the correct output, etc. It becomes harder and harder to reason why a test failed or what a test is actually testing if a test starts checking for too many different things.

There are a couple of other places that might have this issue. Happy to discuss this more if needed.

kemccusker · 2023-04-18T18:58:59Z

tests/test_preprocessing.py

+
+def test_sum_AMEL(tmp_path):
+    """
+    Test that sum_AMEL outputs a Zarr file returns a file with damage function coefficients summed


@JMGilbert is this doc string correct? I thought it would be summing spatial damages, not df coefs.

Oops yeah this is wrong

kemccusker · 2023-04-18T19:01:33Z

tests/test_preprocessing.py

+        dummy_soecioeconomics_file, consolidated=True, mode="w"
+    )
+
+    if batchsize != 15:


is this block actually getting tested?

Yup, one of the pytest fixtures makes batchsize equal to 5 which triggers this conditional.

Oh jeez I actually checked the fixture and saw only 15s. Obviously I misread it. Thanks!

kemccusker · 2023-04-18T19:12:05Z

tests/test_preprocessing.py

+        )
+    )
+
+    if reduction not in ["cc", "no_cc"]:


@JMGilbert this is another spot where the test is testing two things. This conditional testing of the error should be another test function.

For sure -- I was somewhat reluctant to separate a few of these tests because the conditional here still takes some of the synthetic inputs that I generate above this line. I was considering doing a lot of generalization (because I learned about how to make tests share inputs later on), but wasn't sure how much extra time to put in to clean these tests up given that the functionality won't actually change. I could also just copy and paste the synthetic inputs and this conditional into a new test but that feels wrong.

(happy to do either, just wanted to make sure that you want the time put in on this)

I see what you mean. I think going the easy route and copying the synthetic inputs to another test func is probably fine - what do you think @brews ?

@kemccusker I think spare copy-and-paste is fine for an early test for legacy code — like, don't make it too much of a habit. Doing a nice pytest fixture to help setup input data or something might be a better way of doing it, but it's not worth the effort if we do it sloppy or if we're likely to turn around and refactor the code we are testing anyways. I'd let y'all be the judge of that and if its worth cleaning up the test so you can read its intentions later. I would argue the important thing for now, and this PR, is to get some kind of test in place for this code/behavior.

ok @JMGilbert let's go w/ a new test the "easy" way for now since it should be quick, keeping in mind Brewster's advice for future refactors.

kemccusker · 2023-04-18T19:13:07Z

tests/test_preprocessing.py

+            ce_batch_coords=ce_batch_coords,
+        )
+
+        if not zero or reduction == "no_cc":


could also consider splitting these two cases into two tests.

JMGilbert · 2023-04-24T15:18:48Z

@kemccusker I made the tests case by case in all of the places where it makes sense. The only remaining test that has a meaningful if/else is testing the same line of code in both conditions, so I think it makes sense to keep it like that. Should be good to go now.

kemccusker

Awesome, looks good to merge, @JMGilbert ! Thanks for all the effort!

Add pytests for USA subsets and sum_AMEL

03d22c4

JMGilbert added 6 commits January 20, 2023 14:33

Add tests for reduce_damages

e28338d

black

b01546d

Fix flake8 errors

5af02f9

Add test for ce_from_chunk

dd6410a

Remove deprecated

0d926fa

Replace deprecated function

cda25bc

JMGilbert commented Jan 23, 2023

View reviewed changes

JMGilbert marked this pull request as ready for review February 7, 2023 16:55

JMGilbert changed the title ~~Add pytests for USA subsets and sum_AMEL~~ Add pytests for preprocessing Feb 10, 2023

JMGilbert added 3 commits February 10, 2023 14:22

Merge branch 'dscim-v0.4.0' into preprocessing_pytests

1639416

black

0a50992

Remove deprecated reformat_climate_files + reformat.py

4a5159a

JMGilbert mentioned this pull request Feb 16, 2023

Climate reformatting to be removed #71

Open

JMGilbert added 2 commits February 16, 2023 16:17

Update CHANGELOG.md

8a33c67

Merge branch 'dscim-v0.4.0' into preprocessing_pytests

07cc221

JMGilbert requested review from davidrzhdu and kemccusker February 16, 2023 22:20

Fix typo in CHANGELOG.md

1f043a3

kemccusker reviewed Apr 11, 2023

View reviewed changes

tests/test_preprocessing.py Show resolved Hide resolved

tests/test_preprocessing.py Show resolved Hide resolved

tests/test_preprocessing.py Outdated Show resolved Hide resolved

tests/test_preprocessing.py Outdated Show resolved Hide resolved

kemccusker requested review from brews and removed request for brews April 11, 2023 00:54

JMGilbert added 4 commits April 11, 2023 10:58

Add US territories to testing data

8136d27

Merge branch 'dscim-v0.4.0' into preprocessing_pytests

af5c731

Merge branch 'preprocessing_pytests' of https://github.com/ClimateImp…

a07cc3b

…actLab/dscim into preprocessing_pytests

Black

7e6b438

brews reviewed Apr 11, 2023

View reviewed changes

JMGilbert added 6 commits April 12, 2023 14:51

Correct dummy us territory region in testing

b18d500

black

6a1cb3d

black

4b1041b

Add docstrings to testing functions

5a29d61

Moved some test functionality to new function

d581eb9

Update CHANGELOG.md

49bc591

kemccusker reviewed Apr 18, 2023

View reviewed changes

Correct test_sum_AMEL docstring

b51a177

kemccusker mentioned this pull request Apr 20, 2023

Pytests for RFF #73

Merged

JMGilbert added 3 commits April 20, 2023 15:39

Separate error cases in preprocessing pytests

e3196cb

Fix fixtures

5804e74

Fix Flake8

e0244d8

kemccusker approved these changes Apr 24, 2023

View reviewed changes

kemccusker merged commit 58fdca2 into dscim-v0.4.0 Apr 25, 2023

kemccusker deleted the preprocessing_pytests branch April 25, 2023 00:11

		xr.testing.assert_equal(ds_out_expected, ds_out_actual)


		def test_sum_AMEL(tmp_path):

Conversation

JMGilbert commented Jan 20, 2023

Uh oh!

codecov-commenter commented Jan 20, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JMGilbert commented Jan 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JMGilbert commented Feb 16, 2023

Uh oh!

kemccusker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brews left a comment

Choose a reason for hiding this comment

Uh oh!

brews Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brews Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JMGilbert commented Apr 24, 2023

Uh oh!

kemccusker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jan 20, 2023 •

edited by codecov bot

Loading

brews Apr 11, 2023 •

edited

Loading

brews Apr 11, 2023 •

edited

Loading