Use the seed in all rng in blending code #449
Conversation
|
@mats-knmi Did you make any other changes but this issue? It is on my TO-DO list to run both the old and new versions of the code to compare the outputs to make sure that they provide the same output (which will be difficult from what I understand...). |
|
In this PR I only made the changes I described (only this commit: 07fbeb0). Indeed the current blending code is non-deterministic even if you provide a seed. We could apply this fix to the master branch separately and then also to your refactor branch. Then it should be the case that outputs from 2 runs are identical if you did not make any functional changes. |
|
I didn't want to open a PR for any changes to the blending code before you got around to merging your PR, because I expect it would be hard to merge any new changes back into your refactored code. But if it helps you out in validating we could do that. |
|
I think it might be best to add your changes to my PR and then make the changes to the master so I can compare them properly. |
mats-knmi
changed the base branch from
master
to
refactor-steps-in-blending-code
mats-knmi
changed the title
|
Ok I will open a new PR for master then. You can merge this PR into your branch when you're ready. I updated the target branch to yours so if you click merge it will go into your branch. |
|
Oh wait, I noticed that the tests have issues with the new code. I will update them in this PR still |
|
I commited the last fixes so it should be good to go to be merged into your branch @sidekock |
|
And I opened a new PR for the master branch: #450 |
* Refactored all names in the steps blending code from old to new * Made some name changes but test still do not pass * Fixed naming changes, now the tests pass * Built the rough scaffolding for the blending class * Refactored untill no rain case * Added code to estimation of ar parameters of radar * Next go, start with forecast loop #7 * Added some uniformity between nowcast and blending steps. Now at # 8.4 for the refactoring * Small changes since prev commit * All code is tranfered. Last part of the main loop needs to be refactored * Everything is refactored, no test ran as of yet * Old forecast function is updated to fit newly refactored code * Removed old code which is no longer used * 6 more tests that fail * All tests pass, still need to fix TODOs * Updated gitignore * Cleanup of params and state dataclasses, next step: better typing * Cleanup of params and state dataclasses, now all tests pass * Added correct typing to all parts of params and state * Ready for pull request * Made changes for Codacy review * Added aditional tests which currently fail in master branch * Update .gitignore Co-authored-by: mats-knmi <145579783+mats-knmi@users.noreply.github.com> * Used the __zero_precip_time in __zero_precipitation_forecast() * Changed typing hints to python 3.10+ version * Added comments back to the State dataclass * Changed the self.__state.velocity_perturbations = [] to self.__params.velocity_perturbations = [] in __initialize_random_generators * Added code changes as suggested by Ruben, comments and documentation to come later * Added frozen functionality to dataclasses, removed reset_state and fixed seed assingments * Added frozen dataclass to nowcast * The needed checks are done for this TODO so it can be removed * Use the seed in all rng in blending code (#449) * Use seed for all rng to make a test run completely deterministic * fix probmatching test and some copy paste oversights * Add test for vel_pert_method * Change the test so that it actually runs the lines that need to be covered * Removed deepcopy of worker_state. The state is now accessable to all workers at the same time * Update to probmatching comments to keep in track with main * Fix for multithreading issue, this produces exactly the same results as the master * Added additional documentation * Bump version * Updates some files that do not pass the new black version * Updated examples to work with new black version --------- Co-authored-by: mats-knmi <145579783+mats-knmi@users.noreply.github.com>
In some places in the blending code the rng was done without using the provided seed. This meant that a run with a specified seed did not have a completely deterministic outcome and this makes testing harder. This PR fixes this.
NOTE: This PR is based on the refactor branch from @sidekock, so that branch needs to be merged first before this can be merged.
The only changes I added are in this commit: 07fbeb0.