cal-adapt / climakitae

A Python toolkit for retrieving, visualizing, and performing scientific analyses with data from the Cal-Adapt Analytics Engine.
https://climakitae.readthedocs.io
BSD 3-Clause "New" or "Revised" License
21 stars 2 forks source link

Unit conversion happening twice #418

Closed nicolejkeeney closed 2 months ago

nicolejkeeney commented 2 months ago

Description of PR

Looks like unit conversion was happening two times in the data retrieval code. An error was being raised for monthly precip data retrieval in the first conversion because the frequency attribute had not yet been added by that time in the workflow, raising an error for unit conversion.

I'm not sure if this may raise other issues down the line. I tested a few conversions and it seems fine but maybe there are things I'm not thinking of. Time will tell...

Type of change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Checklist:

nicolejkeeney commented 2 months ago

Assessing precip units issue

claalmve commented 2 months ago

I looked into the call stacks of this PR and cksplit2a when retrieving precip data, and this fix makes sense with me! It looks like the extra convert_units is happening on each individual dataset before they're merged together, and frequency is only added after the merge, so this fix makes sense to fix the issue at hand. What I'm not too sure about is why this only happens with precip retrieval and not other variables. Otherwise, LGTM!

nicolejkeeney commented 2 months ago

I looked into the call stacks of this PR and cksplit2a when retrieving precip data, and this fix makes sense with me! It looks like the extra convert_units is happening on each individual dataset before they're merged together, and frequency is only added after the merge, so this fix makes sense to fix the issue at hand. What I'm not too sure about is why this only happens with precip retrieval and not other variables. Otherwise, LGTM!

Thanks for checking the logic!! I'll merge it now