search

ClimateImpactLab / dodola

Containerized application for running individual tasks in a larger, orchestrated CMIP6 bias-adjustment and downscaling workflow.
https://climateimpactlab.github.io/dodola/
Apache License 2.0
14 stars 7 forks source link

fix bad references #178

Closed emileten closed 2 years ago

emileten commented 2 years ago

This fixes a few object reference mistakes in dodola.core.standardize_gcm, that were causing https://github.com/ClimateImpactLab/downscaleCMIP6/issues/439 but could have been in general more problematic.

In the beginning of the function, a new transformed dataset object named ds_cleaned is created out of ds, but in subsequent calls, there are remaining references to ds where we should reference the transformed object instead.

This fix mechanically solves /ClimateImpactLab/downscaleCMIP6/issues/439 (which I verified in this workflow : https://argo.cildc6.org/archived-workflows/default/6da813ba-47ba-45a0-95d8-36444e2bcf7b), because we actually drop the time bounds in ds_cleaned, which were causing the issue.

brews commented 2 years ago

I'm a little suprised that unit testing did catch this. Do we actually have a test checking that calendars get converted in standardize_gcm? This might be something to come back to later, as it sounds like an important behavior.

emileten commented 2 years ago

@brews, you mean that you're surprised it did not catch it, right ?

I don't fully understand your comment : we do have unit tests for calendar conversion. The 'error' I am fixing here wasn't as consequential as you might think, the calendar was still getting converted. Bad referencing meant that one step at the beginning of the code (where, among other things, we drop time_bnds) was not being taken in account. Since that step isn't 'key', things were in general working.

brews commented 2 years ago

@emileten Yeah, I was expecting a larger impact from the bug but I guess you're right. Thanks for the clarification.