noaa-ocs-modeling / EnsemblePerturbation

perturbation of coupled model input over a space of input variables
https://ensembleperturbation.readthedocs.io
Creative Commons Zero v1.0 Universal
7 stars 3 forks source link

Update test due (partially) to updated `stormevents` #94

Closed SorooshMani-NOAA closed 1 year ago

SorooshMani-NOAA commented 1 year ago

@WPringle I have two commits for this PR. One is just field width update due to stormevents (as expected), but the other one is something I was not expecting and I'm not sure how/when the failure started showing up. Basically the result of perturbation is a little different, can you please inspect https://github.com/noaa-ocs-modeling/EnsemblePerturbation/pull/94/commits/307074281ec76a06583180dfe20f4854dbae03d1 and see if you can find anything suspicious to fix before merging updated test references?

The updated files are related to this test: https://github.com/noaa-ocs-modeling/EnsemblePerturbation/blob/e7d9ec735b68249e69d01051a58b84838e67f6b8/tests/test_track_ensemble.py#L83-L104

Note that this PR is based on main branch, and it does not have any of your updates for GAHM.

codecov-commenter commented 1 year ago

Codecov Report

Merging #94 (0d928e8) into main (e7d9ec7) will increase coverage by 0.06%. The diff coverage is 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   19.15%   19.22%   +0.06%     
==========================================
  Files          26       26              
  Lines        3581     3584       +3     
==========================================
+ Hits          686      689       +3     
  Misses       2895     2895              
Impacted Files Coverage Δ
tests/test_track_ensemble.py 98.33% <100.00%> (+0.08%) :arrow_up:

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

WPringle commented 1 year ago

@SorooshMani-NOAA For what reason did the json change? I don't see any change in the test code. But random suggests that those values would be randomly chosen and so would differ with each call. Not sure why we would have random in the tests.

WPringle commented 1 year ago

@SorooshMani-NOAA OK, I see I think that's not right to use random in the test suite. Need to change to use the korobov sequence or something.

SorooshMani-NOAA commented 1 year ago

@WPringle you're right. The test has not changed since last year. I think since the test is done based on pseudorandomness and not real randomness, the results remained the same, but now for some reason it changed. Do you have any suggestion for updating the results or the test? Should we just go through with this updated ref for now?

Updated Ok, so I'll update the test with a korobov and add new ref files for it

WPringle commented 1 year ago

@SorooshMani-NOAA That's strange how it never changed.. the random method should be properly random unless it is seeded using the same value.

SorooshMani-NOAA commented 1 year ago

@WPringle I just realized that that specific test actually never checks the resulting files against the reference files, so even if it changes, it won't complain! The reason I captured the changes was that I copied all update files from the output directory to reference directory after running the test.

So basically the test is: "_Is this function perturb_tracks running without errors?_" Given that, do you think it is OK to leave it as such for now or do you think it'd be better if we use korobov sequence instead of random and check the resulting files against the refs?

WPringle commented 1 year ago

@SorooshMani-NOAA I think it's ok then to leave it and just to check it's working. But we don't need reference files then I guess. Just that the files and their filenames are output correctly when the test is run.