xhydro icon indicating copy to clipboard operation
xhydro copied to clipboard

First draft of stationary extreme value analysis module

Open Sci-pio opened this issue 1 year ago • 6 comments

Pull Request Checklist:

  • [ ] This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • [x] (If applicable) Documentation has been added / updated (for bug fixes / features).
    • There is an example notebook at docs/notebooks/extreme_value_analysis.ipynb
  • [x] (If applicable) Tests have been added.
    • All the main parameter estimation functions in parameterestimation.py have been tested in tests/test_extremes
  • [ ] CHANGELOG.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?

A new subpackage called extreme_value_analysis has been added. This subpackage is a python wrapper around the julia Extremes.jl package.

As I am leaving, all the information about what needs to be done to finish this project can be found on 'https://docs.google.com/document/d/1Jg5_xQoyyekLfg8sfDff5-XGAzomPE8hSmA_VJZAKMo/edit?usp=sharing' (in French). @ospinajulian will be taking care of the julia wrapper from now on!

Does this PR introduce a breaking change?

Not to my knowledge.

Other information:

Need to add juliacall to the testing environment on GitHub for tests to pass.

Sci-pio avatar Jul 22 '24 13:07 Sci-pio

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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 ✨

github-actions[bot] avatar Jul 22 '24 14:07 github-actions[bot]

Really strange that test test_gpfit, test_gpfitpwm and test_gpfitbayes aren't passing because they pass without a problem on my machine. Investigating..

Sci-pio avatar Jul 24 '24 13:07 Sci-pio

All tests and notebooks passed, however I suspect there is some flaky behavior. We should probably wait until the pipeline passes a few times in a row

Sci-pio avatar Jul 24 '24 14:07 Sci-pio

For ReadTheDocs, it's failing because you need to specify whether you want docs/notebooks/extreme_value_analysis.ipynb in the documentation (add it to an index.rst) or not (add it to ignored in docs/conf.py).

Zeitsperre avatar Jul 24 '24 14:07 Zeitsperre

First complete pipeline pass

Sci-pio avatar Jul 24 '24 18:07 Sci-pio

@Zeitsperre @RondeauG @sebastienlanglois I’ve already done what Kamil mentioned was missing. Also, I included some new features from Extremes.jl (e.g., confidence interval for the return levels) and added some of the comments from this PR. There are still a few details to improve and some issues to fix to pass the test. Even so, I’d appreciate your feedback at this point to improve anything that still needs work.

ospinajulian avatar Nov 20 '24 21:11 ospinajulian

Je ne suis pas allé entièrement en détail, mais à première vue ça me semble très clean.

On avait discuté de l'idée de peut-être rendre ça seamless avec xh.frequency_analysis.local. Est-ce que ça te semble encore réaliste, maintenant que tu as regardé le code ? (Si oui, ça serait probablement dans une PR séparée).

Yes I think it's possible. My first idea would be to set the distributions and methods of Extremes.jl as default to return the confidence interval and the non-stationary results. If the user wants to include another combination of dist and method, the function can computes the parameters with local.fit() and return NaNs for the confidence intervals. For the non-stationary it would return NaNs if the user tries to use a dist that is not in Extremes.jl.

ospinajulian avatar Dec 05 '24 19:12 ospinajulian

@ospinajulian This seems to be (more or less) working now.

The issues I can see are:

  • The notebooks use a lot of RAM and time, more than is available to use in ReadTheDocs. We might need to constrain the examples a bit more.
  • The Julia version used in the CI is the one installed on the VM (not via conda). I think there may be some permissions issues or environment settings set by JuliaCall that don't translate well to conda-based Julia installations. Oddly enough, this works fine locally. Caveat emptor.
  • The tests that require Julia should be marked as such (with an accompanying marker definition under pyproject.toml's tool.pytest.ini_options). This isn't the end of the world, though.

I'm pushing some final changes. The minimum that needs to be dealt with are the docs. Good work!

Zeitsperre avatar Dec 12 '24 18:12 Zeitsperre

@ospinajulian The docs failures ([codeautolink.match_name]) are expected. I'll push a fix tomorrow!

Zeitsperre avatar Jan 09 '25 21:01 Zeitsperre

@ospinajulian If this Pull Request is ready, I would click the "Ready for review" button and let people know.

There are a lot of changes here, so maybe mentioning the files that require more attention could help us be more strategic with the review.

Zeitsperre avatar Jan 21 '25 21:01 Zeitsperre

@ospinajulian If this Pull Request is ready, I would click the "Ready for review" button and let people know.

There are a lot of changes here, so maybe mentioning the files that require more attention could help us be more strategic with the review.

The file that requires the most attention is the one importing Julia (julia_import.py), as well as the changes I made in environment.yml, environment-dev.yml, and pyproject.toml. I would also appreciate it if you could take a look to files that implement the wrapper (i.e., paramestimation.py and util.py).

ospinajulian avatar Jan 22 '25 01:01 ospinajulian