Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the seed in all rng in blending code #449

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

mats-knmi
Copy link
Contributor

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.

@sidekock
Copy link
Contributor

sidekock commented Jan 2, 2025

@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...).

@mats-knmi
Copy link
Contributor Author

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.

@mats-knmi
Copy link
Contributor Author

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.

@sidekock
Copy link
Contributor

sidekock commented Jan 2, 2025

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 mats-knmi changed the base branch from master to refactor-steps-in-blending-code January 2, 2025 15:46
@mats-knmi mats-knmi changed the title Draft: Use the seed in all rng in blending code Use the seed in all rng in blending code Jan 2, 2025
@mats-knmi
Copy link
Contributor Author

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.

@mats-knmi
Copy link
Contributor Author

Oh wait, I noticed that the tests have issues with the new code. I will update them in this PR still

@mats-knmi
Copy link
Contributor Author

I commited the last fixes so it should be good to go to be merged into your branch @sidekock

@mats-knmi
Copy link
Contributor Author

And I opened a new PR for the master branch: #450

@sidekock sidekock merged commit 47ab6c3 into refactor-steps-in-blending-code Jan 2, 2025
@mats-knmi mats-knmi deleted the blending-seed-fixes branch January 2, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants