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

Optimal interpolation #88

Merged
merged 81 commits into from
Mar 21, 2024
Merged

Optimal interpolation #88

merged 81 commits into from
Mar 21, 2024

Conversation

Mayetea
Copy link
Collaborator

@Mayetea Mayetea commented Feb 11, 2024

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • CHANGES.rst has been updated (with summary of main changes).
    • Link to issue (:issue:number) and pull request (:pull:number) has been added.

What kind of change does this PR introduce?

Ajout d'outils pour l'interpolation optimale dans xhydro.

  • Outil de validation croisée de fichiers Hydrotel contenant des débits observés et des débits simulés
  • Outil de comparaison entre l'interpolation et les débits fournis

Does this PR introduce a breaking change?

No.

Other information:

Je n'ai pas modifié le fichier changes.rst, puisque je n'étais pas sur de ce que je devais y mettre.

Mayetea and others added 28 commits August 25, 2023 16:59
Le script Matlab Compare_result est converti en Python, mais a besoin d'être optimisé
Optimize compare_result script
Le code de comparaison des résultats a été optimisé et commenté.
This commit have the optimal_interpolation working but without refactoring and optimization
Add write_final_result in netCDF and change the obj_funcs to point on the new branch
Copy link

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

  • The relevant author information has been added to AUTHORS.rst and .zenodo.json.

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

@RondeauG
Copy link
Collaborator

@Mayetea PTI, tu peux faire pre-commit install dans ton répertoire local de xhydro pour activer les fonctionnalités de pre-commit et régler les problèmes de formatage. C'est ce qui fait planter les premiers checkups.

Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

On s'approche !

tests/optimal_interpolation/test_integration.py Outdated Show resolved Hide resolved
tests/optimal_interpolation/test_integration.py Outdated Show resolved Hide resolved
tests/optimal_interpolation/test_integration.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/utilities.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/utilities.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/compare_result.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/cross_validation.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/optimal_interpolation_fun.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/optimal_interpolation_fun.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/optimal_interpolation_fun.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

Quelques commentaires mineurs, mais sinon ça me semble prêt ! Merci pour le gros refactoring, ça me semble avoir été pas mal de job.

environment-dev.yml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/ECF_climate_correction.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/ECF_climate_correction.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/optimal_interpolation_fun.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/optimal_interpolation_fun.py Outdated Show resolved Hide resolved
xhydro/optimal_interpolation/utilities.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Mar 21, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the notebooks Run tests against notebooks label Mar 21, 2024
@richardarsenault
Copy link
Contributor

@Zeitsperre

The testdata fetching function need to be rewritten to work with the new system. I'd suggest to first add them into the main branch of hydrologie/xhydro-testdata, then add the necessary info to registry.txt before fetching the files with DEVEREAUX.fetch("..."). Let me know if you need further guidance on how to do this.

Does this still apply, with the new system with pooch?

@Zeitsperre
Copy link
Collaborator

@richardarsenault

Not anymore. The fetching code has migrated to pooch which is good enough for right now. A next step would be to implement the mechanism I detailed, but that will come later.

For context, the goal of pooch and the catalogue is to reduce the potential for breakage when fetching remote files, and to reduce the duplicated code and maintenance required (the original testdata fetching code is the same in at least five projects we maintain). Getting that to work with things like pytest-xdist (for asynchronous testing) is really finicky as well. This is already a major improvement.

@Zeitsperre Zeitsperre self-requested a review March 21, 2024 18:15
@richardarsenault
Copy link
Contributor

OK great, thanks, great to hear!

@richardarsenault richardarsenault merged commit 00825cf into main Mar 21, 2024
38 checks passed
@richardarsenault richardarsenault deleted the optimal-interpolation branch March 21, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests notebooks Run tests against notebooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants