search

USDA-ARS-NWRC / smrf

SMRF was designed to increase the flexibility of taking measured weather data, or atmospheric models, and distributing the data across a watershed.
Other
12 stars 4 forks source link

Tests - Gold files - Fix scripts and update cleanup logic. #203

Closed jomey closed 3 years ago

jomey commented 3 years ago

Needed to update the SMRF files locally when running with the latest for topolib and found a few paths that needed updating.

jomey commented 3 years ago

The build is failing with an installation problem for the package. I had the same issue locally when it tried to compile with gcc instead of cython. The Cython version still compiles fine. I could not find a quick fix for the gcc issues.

scotthavens commented 3 years ago

Looks like it's failing because the numpy header files are not included when compiling. The line in setup.py confirms you're issue with it working in cython but not gcc. Probably removing the if USE_CYTHON: will work with gcc compiling as well?

jomey commented 3 years ago

Looks like it's failing because the numpy header files are not included when compiling. The line in setup.py confirms you're issue with it working in cython but not gcc. Probably removing the if USE_CYTHON: will work with gcc compiling as well?

I will give it a shot.

jomey commented 3 years ago

Tests are failing with the udpated/fixed topocalc calculations.

You OK with me updating the gold files with this PR?

scotthavens commented 3 years ago

Yes, update the gold files and we'll probably want to pin topocalc to >0.5.0?

After you update the gold files, you can run gold_difference in the scripts folder and upload the images here if they are different. Should just be net_solar and thermal that are different.

jomey commented 3 years ago

Yes, update the gold files and we'll probably want to pin topocalc to >0.5.0?

After you update the gold files, you can run gold_difference in the scripts folder and upload the images here if they are different. Should just be net_solar and thermal that are different.

I would run the script, but what happened to goldmeister? https://github.com/USDA-ARS-NWRC/goldmeister => 404

jomey commented 3 years ago

To show the only differences based of the failing tests, here the previous failure: https://travis-ci.com/github/USDA-ARS-NWRC/smrf/jobs/500501641

All errors are on net_solar and thermal.

scotthavens commented 3 years ago

Try now, made it private by mistake.

scotthavens commented 3 years ago

To show the only differences based of the failing tests, here the previous failure: https://travis-ci.com/github/USDA-ARS-NWRC/smrf/jobs/500501641

All errors are on net_solar and thermal.

Great, those should have been the only changes. Looks like they are quite small changes as well. Will be good to document with the gold difference plots.

jomey commented 3 years ago

Last place failing and to be expected is: https://github.com/USDA-ARS-NWRC/smrf/blob/master/smrf/tests/envphys/solar/test_toporad.py

For the tests, are we really that confident to compare on floating point precision? I suggest that the test should pass ignoring any precision since it is a result based of multiple calculations with uncertainty themselves that I don't see have that high of a precision from the very first one.

scotthavens commented 3 years ago

Forgot about that one... Those gold values that we're comparing against will have to change at some because of the update to topocalc.

The objective of these tests were to ensure that the calculations for stoporad are correct. The best way to do this would be to generate gold files for these tests and spatially compare them. The fastest method was just to take the min/max/mean and compare. So yes, floating point isn't probably needed in the way it's implement but it should test against some sort of spatial gold file.

How about for now, decimal=0 and #204 will addressing a better method for testing this.

jomey commented 3 years ago

Gold Differences

Only showing the files that changed for RME and Lakes

Station RME

net_solar nc_net_solar thermal nc_thermal

HRRR RME

net_solar nc_net_solar precip_temp nc_precip_temp thermal nc_thermal vapor_pressure nc_vapor_pressure wind_direction nc_wind_direction

HRRR Lakes

net_solar nc_net_solar precip_temp nc_precip_temp thermal nc_thermal vapor_pressure nc_vapor_pressure

scotthavens commented 3 years ago

Interesting that it's different for vapor pressure, precip temperature and wind. I'm going to chalk that up to you running the gold files on you're computer instead of actual differences with the calculation (small values anyways). All those use compiled C code which may be slightly different on Joe's newer machine than ours.

You can see the topocalc bug fix in the Lakes net_solar. Those vertical lines are the artifacts noted in topocalc. Interesting to see that thermal in the Lakes also showed a bit of difference along the ridges. topocalc is now looking farther in the distance for the horizon so they must be finding different horizon's and changing the sky view factor.