watem-sedem / rfactor

R-factor
https://watem-sedem.github.io/rfactor/
GNU Lesser General Public License v3.0
6 stars 2 forks source link

Python core - add additional unit tests #28

Closed stijnvanhoey closed 2 years ago

stijnvanhoey commented 3 years ago

@Sachagobeyn, as I do not have the user rights to adjust the open PR directly (it is not my original PR anymore), this PR adds a number of unit tests to the open PR python-core:

The second part is the shortest in implementation, but the most important one. However, based on https://github.com/cn-ws/rfactor/pull/26#issue-712335495 it is for me not clear which of the files tests/data/testdata_maximum_intensity.csv and/or tests/data/testdata_maximum_intensity_matlab_clone.csv were created by which scripts/routine, which is the Matlab benchmark output to compare with to check the correct implementation and what should be the difference between both reference files?

Apart from that, there were some elements in the example notebook, e.g. cwd = Path().resolve() that should not be there, please verify that the adjustments were correct and appropriate.

stijnvanhoey commented 3 years ago

With respect to the current benchmark implementation, we still have a difference in the unit test for ("P05_039", 2017) which is due to the inclusion/exclusion of the event_rain_cum equal to 1.27 with some float approximation difference. I did added the round(rain_events["event_rain_cum"], 2) > event_threshold - round - to the comparison to solve a similar issue in ("P01_003", 2020) where this value was added in my calculation, but not in the benchmark data set. However, in this case it is vice-versa and there should be a clear way of handling these boundary-values (triggered by numerical approximation).

I left the round in the code, but I leave it to you to decide on how to handle this.

Note both 'troubleshooters' are part of the parameterized version of the benchmark for easier comparison, see test test_rfactor_benchmark_single_year

Sachagobeyn commented 2 years ago

Hi Stijn,

Thanks for the help,

Things I'll tackle before going to release