OpenSenseAction / poligrain

Simplify common tasks for working with point, line and gridded sensor data, focusing on rainfall observations.
https://poligrain.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2 stars 10 forks source link

Add functions for plotting CML metadata #40

Closed bwalraven closed 3 days ago

bwalraven commented 3 months ago

Adding a function to make frequency vs. length plots.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (61c4063) to head (08e0191). Report is 21 commits behind head on main.

:exclamation: Current head 08e0191 differs from pull request most recent head 1715643

Please upload reports for the commit 1715643 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #40 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 3 5 +2 Lines 43 269 +226 ========================================== + Hits 43 269 +226 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bwalraven commented 1 week ago

I added a simple notebook example to complete the issue for now. It runs without errors locally and I was able to merge my branch with the main that I forked, but somehow the checks above are not successful. Not sure how to interpret or fix the failed checks. It looks the same as #58. @cchwala perhaps you know?

cchwala commented 1 week ago

I ran into the same errors during CI runs, see my latest comments and commits in #41.

You have to configure pytest so that it ignores the ResourceWarning. You should do it like I did here

https://github.com/OpenSenseAction/poligrain/blob/cc6babfa3f75a9a5f0fe7673cae6eac0f23dfd42/pyproject.toml#L83

cchwala commented 1 week ago

@bwalraven Are there some things you want to add to this PR before it get's merged? Since no other PR currently depends on your PR, you could still use the time till the meeting next week to add some things you have in mind. It would be enough to merge next Tuesday. But I can also merge now.

Also note, that after I merged #41 you now have a conflicting pyproject.toml because I also added the config to ignore the pytest warnings. You either need to merge main or manually apply the exact same changes.

bwalraven commented 1 week ago

Okay, I'll try to add some functionalities before Tuesday then and give a heads up when you can merge.

bwalraven commented 3 days ago

@cchwala I added a few other functions to plot some meta data. It passed all the tests so it should ready for merge.

I did have to go back and forth between GH and my local set up to fix some errors that didn't show up when I ran it locally, but raised an error when wanting to merge on GH. Any ideas how to fix this? Or make my set up such that the errors are raised locally as well? This would allow me to catch errors before pushing to GH every time.

cchwala commented 3 days ago

Any ideas how to fix this? Or make my set up such that the errors are raised locally as well?

The RuntimeWarning from matplotlib because of too many open figures maybe depends on local config. So it is different on CI and you machine. Anyway it is just a warning. But warning let tests fail, unless explicitly allowed.

I am not sure why the assert all(isinstance(patch, PathCollection) for patch in patches) passes on your machine but failed on CI. This might stem from using Python version < 3.9 on your machine, but I am not sure.

It is indeed annoying to debug failing test that only fail on CI... We can have a look at your setup tomorrow so see if I can spot something that can be improved.

cchwala commented 3 days ago

I just added some minor changes and merge it now. Thanks for this extended contribution 👍