-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fixed random seed application in Spotpy #300
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le code change me semble correct.
Mais j'ai une question, on dirait que les outputs ont tous été enlevé du nb original. C'est normal? Maintenant on veut les garder?
J'ai rien contre garder les outputs. Je préfère les garder même. Je fais juste remarquer le changement de façon de faire. Est-ce que ça risque de surprendre quelqu'un?
@@ -39,9 +39,41 @@ | |||
}, | |||
{ | |||
"cell_type": "code", | |||
"execution_count": null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pourquoi ce code cell n'a pas été exécuté avant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui, on avait tout retiré les outputs mais je pense que c'est correct de les garder car il s'agit d'un NB démo qu'on roule dans l'article, alors ça ne change rien (comparativement aux autres NB dans lesquels on veut que les usagers comprennent mieux les étapes pour les répéter dans leurs propres NB).
Merci de l'avoir souligné par contre!
test_spotpy_calibration fail 2 fois ! Drole de coincidence que le changement est pour le spotpy (https://github.com/CSHS-CWRA/RavenPy/actions/runs/5697951328/job/15445290895?pr=300) py39
py310
|
J'ai vu aussi que ça fail, mais hier quand j'ai fait le test ça avait fait un fail avec 3.9 et 3.10 mais pour des deployment différents (ubuntu-latest et ubuntu-latest/Conda). C'est une erreur que j'ai déjà vue avant aussi mais je ne suis pas certain de ce qui la cause! Ça ne devrait pas être en lien avec le NB qui n'a rien changé au code de Spotpy lui-même... |
I'm honestly not sure what the cause of that error is, but I think it has something to do with asynchronous reading of a netcdf file. Feel free to ignore it, it's well beyond the scope of the PR here. |
It's good for me. I already approved the PR anyways. Agreed even if any fixes are required, it's outside the scope of this PR. If you want to remove the warnings in the first cell, I think you'll have to refresh this notebook in the For Again, make sure to refresh notebook output in the official |
OK thanks for the responses! I just tried re-running the NB in the "current" version of the server ("current-stable-version-py39-230601") and I got the same "PYGEOS" warnings. Anything else I should be doing? |
Pull Request Test Coverage Report for Build 5697951328
💛 - Coveralls |
Maybe look at the ordering of at env var?
It might have to be done before some other import.
Also restart the kernel before each run to have a fresh env.
|
for more information, see https://pre-commit.ci
Changed geopandas import order and this fixed the PYGEOS warning! thanks! |
Spotpy does not have a mechanism to set a seed for the RNG. We use np.random.seed to fix a seed in numpy before launching Spotpy. However, the paper NB had the seed set after Spotpy initialization, leading to the seed being fixed by spotpy instead, before we set our own seed, leading to it being useless. This has been fixed.