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

FIX: Beta parameter in GR percolation #158

Merged

Conversation

inoelloc
Copy link
Contributor

@inoelloc inoelloc commented Mar 28, 2024

  • The beta parameter in GR production is time step dependent. Currently, beta was assigned to the daily time step value (i.e., 9/4) This commit adds a beta parameter which is time step dependent. The hydrological module concerned by this modification are gr4, gr5 and loieau

  • This modification impacted the test. A new baseline has been generated. The setup of the tests on mutliset estimate has been changed to overcome warning when dividing by zero.

  • The docstring has been updated

- The beta parameters in GR production in time step dependent. Currently, beta
was assigned to the daily time step value (i.e., 9/4)
This commit adds a beta parameter which is time step dependent. The hydrological
module concerned by this modification are ``gr4``, ``gr5`` and ``loieau``

- This modifications impacted the test. A new baseline has been generated.
The setup of the tests on mutliset estimate has been changed to overcome
warning when dividing by zero.

- The docstring has been updated
@inoelloc inoelloc added the bug Something isn't working label Mar 28, 2024
@inoelloc inoelloc requested a review from nghi-truyen March 28, 2024 13:40
@inoelloc
Copy link
Contributor Author

@nghi-truyen, il y a une erreur avec le test sur multiset estimate. J'ai du changer ce test parce qu'en modifiant la valeur de beta pour la percolation, le test renvoyait une erreur avec une division par zero de var_param quelque chose. En modifiant en local, j'avais plus les warnings et les tests passaient mais la sur la CI, ca ne passe pas. Est ce que tu aurais une idée pour modifier ces tests ?

@nghi-truyen
Copy link
Member

@nghi-truyen, il y a une erreur avec le test sur multiset estimate. J'ai du changer ce test parce qu'en modifiant la valeur de beta pour la percolation, le test renvoyait une erreur avec une division par zero de var_param quelque chose. En modifiant en local, j'avais plus les warnings et les tests passaient mais la sur la CI, ca ne passe pas. Est ce que tu aurais une idée pour modifier ces tests ?

Ah oui, ça peut arriver, ça ne me surprend pas. C'est juste que la taille des samples est trop petite, donc cette erreur peut survenir. Je pense que la solution qui est moins coûteuse serait d'essayer de changer le random_state, sinon il faudra augmenter la taille des samples.

Copy link
Member

@nghi-truyen nghi-truyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finalement !

@inoelloc
Copy link
Contributor Author

Finalement !

Yes :3. Pas tjs simple de changer pas grand chose

@nghi-truyen
Copy link
Member

nghi-truyen commented Apr 2, 2024

@inoelloc Aller je le merge pour pouvoir lancer les calculs avec le nouveau calcul de beta.

@nghi-truyen nghi-truyen merged commit fad6dae into maintenance/1.0.x Apr 2, 2024
18 checks passed
@nghi-truyen nghi-truyen deleted the gr-beta-percolation-time-step-dependent branch April 2, 2024 12:01
@inoelloc inoelloc linked an issue Apr 3, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong percolation value with GR production
2 participants