Convert prsn data from mm to cm by eyvorchuk · Pull Request #113 · pacificclimate/climate-explorer-data-prep

eyvorchuk · 2020-05-11T23:59:02Z

The generate_climos.py and generate_prsn.py files have been modified to convert the prsn data from mm to cm (equivalently, kg/m2 to g/cm2). The test_generate_prsn.py file has an additional assert statement to ensure the correct units.

nikola-rados

Just a few styling comments, otherwise looks good @eyvorchuk!

nikola-rados · 2020-05-12T17:53:37Z

dp/generate_climos.py

+                logger.info("Converting {} variable to units mm/day".format(flux_var))
+            else:
+                logger.info("Converting {} variable to units cm/day".format(flux_var))


The repo has recently been updated to drop support for python <3.6 so that we have access to f-strings. While it may not make a huge difference here, it can make things easier to read overall.

logger.info(f"Converting {flux_var} variable to units mm/day")

nikola-rados · 2020-05-12T18:02:22Z

dp/generate_prsn.py

    valid_units = [
        Unit('kg / m**2 / s'),
        Unit('mm / s'),
+	Unit('mm / d'),
        Unit('kg / d / m**2'),
-        Unit('kg / m**2 / d')
+        Unit('kg / m**2 / d'),
+	Unit('g / cm**2 / s'),
+	Unit('cm / s'),
+	Unit('cm / d'),
+	Unit('g / d / cm**2'),
+	Unit('g / cm**2 / d')
    ]


It may be helpful to order these in some manner (maybe grouping by mm, cm, g, kg) just to make it easier to follow. You may want to double check with @corviday to ensure we aren't missing any cases here.

nikola-rados · 2020-05-12T18:10:51Z

dp/generate_prsn.py

+        prsn_data = prsn_data * Q_(1.0, prsn_units_from).to(prsn_units_to).magnitude	
+
        output_dataset.variables['prsn'][chunk] = prsn_data


Have you considered skipping the extra variable assignment and assigning directly?

output_dataset.variables['prsn'][chunk] = prsn_data * Q_(1.0, prsn_units_from).to(prsn_units_to).magnitude

jameshiebert

Excellent, way to dive into this code base and tackle the problem! I think that you have essentially added the logic necessary to solve the problem, so you clearly understood what needed to be done. Good work!

I have a couple comments on making the code a little better, and some comments for giving us confidence in the results. And the units are confusing me a bit, so maybe you can answer my question there.

You can push more commits to this branch, and it will update the PR.

jameshiebert · 2020-05-12T18:37:23Z

dp/generate_prsn.py

    valid_units = [
        Unit('kg / m**2 / s'),
        Unit('mm / s'),
+	Unit('mm / d'),


It looks like you're mixing whitespace here... the existing file uses spaces and you've inserted lines with tabs. You'll should conform to what's there already and make it consistent.

jameshiebert · 2020-05-12T18:40:19Z

dp/generate_prsn.py

+	Unit('cm / s'),
+	Unit('cm / d'),
+	Unit('g / d / cm**2'),
+	Unit('g / cm**2 / d')


With all of the valid units that you've added to this list, it's probably worth while to eliminate some redundancy by instantiate the Unit objects in a loop. E.g. define the list as strings first and then loop over them.

valid_units = ['kg / m**2 / s' ...] valid_units = [Unit(x) for x in valid_units]

jameshiebert · 2020-05-12T20:17:20Z

dp/generate_climos.py


-        if units in [Unit('kg / m**2 / s'), Unit('mm / s')]:
-            logger.info("Converting {} variable to units mm/day".format(flux_var))
+        if units in [Unit('kg / m**2 / s'), Unit('mm / s'), Unit('g / cm**2 / s'), Unit('cm / s')]:


An option that would make this more readable is to define each set of units as a variable to use in your conditional. For example:

precip_units = {Unit('kg / m**2 / s'), Unit('mm / s'), Unit('g / cm**2 / s'), Unit('cm / s')} snow_units = {Unit('g / m**2 / s'), Unit('cm / s')} if units in precip_units: ...

jameshiebert · 2020-05-12T20:22:21Z

dp/generate_climos.py

-        if units in [Unit('kg / m**2 / s'), Unit('mm / s')]:
-            logger.info("Converting {} variable to units mm/day".format(flux_var))
+        if units in [Unit('kg / m**2 / s'), Unit('mm / s'), Unit('g / cm**2 / s'), Unit('cm / s')]:
+            if units in [Unit('kg / m**2 / s'), Unit('mm / s')]:


Do you need this conditional? It only changes one thing... the output unit in the log message. But you already have the string for the output unit string below: (units * Unit('s / day')).to_udunits_str(). Save that here, then use it both for your log message and for your attributes. Then the conditional can go away.

See "If Statements are a Code Smell".

jameshiebert · 2020-05-12T20:29:36Z

dp/generate_prsn.py

        prsn_data = np.where(means < freezing, chunk_data['pr'], 0)
+
+        prsn_units_from = ureg.parse_units('kg/m^2/s')
+        prsn_units_to = ureg.parse_units('g/cm^2/s')


Can you tell me more about using this as the target unit? You've changed both the mass (kg -> g) and the area (per square meter to per square centimeter). Is that definitely what we want? And is it definitely correct?

Converting from kg -> g multiplies the data by 1000 and converting from /m^2 -> /cm^2 divides the data by 10000, giving the net effect of dividing the data by 10. Since converting from mm to cm also divides by 10, I think this will work. I can check with Lee when he's back.

jameshiebert · 2020-05-12T20:32:58Z

tests/test_generate_prsn.py

    assert fake_dataset.variables['prsn'].standard_name == 'snowfall_flux'
    assert fake_dataset.variables['prsn'].long_name == 'Precipitation as Snow'
-
+    assert fake_dataset.variables['prsn'].units == 'g cm-2 s-1'


There's not actually any check on the data values in this test, so it's hard to know whether it's doing the conversion correctly. Could you work out what the sum snowfall in this data should be an add that to the test?

I believe the dataset used for test_process_to_prsn results in a prsn array of all zeros since these tests pass.

result = fake_dataset.variables['prsn'][:] result = np.where(result != 0) for array in result: assert len(array) == 0

Is there possibly another dataset I could use to test the conversion?

Oh right, you had mentioned that. Well that's not a great testing dataset, then is it?! :)

Maybe you could throw a few strategic low temperature values into the test data set... it might throw some other tests out of whack, but you could adjust as necessary.

Since we're only concerned about the conversion, would it be ok if I store the pr dataset in two variables, perform the conversion on one of them, and check to make sure that each value in the two datasets differs by a factor of 10? Or would you prefer I modify the data set and tests where needed?

rod-glover · 2020-06-19T16:34:10Z

I have a question: Why convert units? P2A, at least, converts units on the frontend; that is a presentation issue, which is the responsibility of the UI, not of the data backend. Possibly the Marmot may need this, possibly lacking units conversions? (That can be remedied, particularly given the utilities now in P2A.)

eyvorchuk · 2020-06-19T16:39:13Z

I have a question: Why convert units? P2A, at least, converts units on the frontend; that is a presentation issue, which is the responsibility of the UI, not of the data backend. Possibly the Marmot may need this, possibly lacking units conversions? (That can be remedied, particularly given the utilities now in P2A.)

I'm not sure. @corviday wrote the original issue.

corviday · 2020-06-19T17:27:08Z

I forgot I'd written that issue. At the time I wrote it, we did not yet have p2a, just PCEX, so it didn't occur to me that we might do unit conversion in the front end instead.

eyvorchuk requested a review from corviday May 11, 2020 23:59

eyvorchuk changed the title ~~Convert prsn data from cm to mm~~ Convert prsn data from mm to cm May 11, 2020

eyvorchuk force-pushed the issue/71 branch from f9efd08 to 1f1f3bc Compare May 12, 2020 00:04

eyvorchuk linked an issue May 12, 2020 that may be closed by this pull request

prsn data should be in cm, not mm #71

Open

Convert prsn data from mm to cm

5b81f23

eyvorchuk force-pushed the issue/71 branch from 1f1f3bc to 5b81f23 Compare May 12, 2020 17:24

eyvorchuk requested a review from nikola-rados May 12, 2020 17:37

nikola-rados approved these changes May 12, 2020

View reviewed changes

jameshiebert requested changes May 12, 2020

View reviewed changes

Eric Yvorchuk added 2 commits May 13, 2020 15:33

Make Style Changes to dp Files

ad26339

Test Conversion From mm to cm

829df26

eyvorchuk requested a review from jameshiebert May 19, 2020 23:26

		prsn_data = prsn_data * Q_(1.0, prsn_units_from).to(prsn_units_to).magnitude

		output_dataset.variables['prsn'][chunk] = prsn_data

Conversation

eyvorchuk commented May 11, 2020

Uh oh!

nikola-rados left a comment

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!

jameshiebert left a comment

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!

eyvorchuk May 12, 2020 • 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!

rod-glover commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyvorchuk commented Jun 19, 2020

Uh oh!

corviday commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eyvorchuk May 12, 2020 •

edited

Loading

rod-glover commented Jun 19, 2020 •

edited

Loading