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

Better control of warnings for users #1600

Closed
erikvansebille opened this issue Jul 16, 2024 · 7 comments · Fixed by #1672
Closed

Better control of warnings for users #1600

erikvansebille opened this issue Jul 16, 2024 · 7 comments · Fixed by #1672

Comments

@erikvansebille
Copy link
Member

The warnings/logger implementation in parcels is a bit clunky; it would be nice to refactor this with a more advanced(?) library. It would especially be nice if users could control which messages they turn on and off.

For example, for some cases where it is expected that particle sets are empty for some time during the simulation, users may want to silence the ParticleSet is empty on writing as array at time XXXX warning

@andrew-s28
Copy link
Contributor

I'd be able to take this one on, motivated by my recent experiences with the ParticleSet is empty message. I wanted to check in before rewriting it to see if you all had an opinion on what a refactor/rewriting of this would look like.

Currently, the warnings in Parcels are implemented through the logging library. I was curious if there is any interest in transitioning to using the warnings library. This has several advantages, in my opinion:

  • Using the warning library will enable users to set their own level of warning using the warnings context manager, which has the advantage of being consistent with other libraries, such as numpy.exceptions, pandas.errors, and astropy.utils.exceptions.
  • Users explicitly managing their own warnings using standard library functionality is better than including another kwarg as input to functions that would manage silencing internally and might go missed by many users. When I tried to suppress Parcels warning in some recent runs where I expected empty ParticleSets, I was surprised that my usual way of handling warnings wasn't working.
  • Warnings could then also be included in documentation with some hints on how to solve them, e.g., a ChunkWarning is issued when chunks are not set optimally, encouraging the user to check the chunking on I/O files.

The reason I wanted to check first is that there is one thing that could be seen as a negative if this is implemented, depending on the goals of the existing logger implementation. The default behavior of the warning library is to only show warnings once, so for the case of the empty ParticleSet warning, the default behavior would change from showing on every iteration to only showing once.

I think this could be mitigated with an example in the tutorials showing how to use the warnings context manager if you desire old behavior (e.g., warnings.simplefilter('always', category=EmptyParticleSetWarning)), and that the overall positives of an updated warnings implementation outweigh the negatives, but I'm curious to see if anyone disagrees before I get working on it.

@VeckoTheGecko
Copy link
Contributor

Thanks for the input here @andrew-s28 ! I agree that using the warnings package is a good direction forward, both from a users pov. as well as aligning ourselves with other project. I was just looking through the codebase, alot of the warnings are logger.warning_once() warnings (even those using logger.warning() are conceptually warning_once). The only exception being the EmptyParticleSetWarning as you mentioned.

@erikvansebille
Copy link
Member Author

Thanks for your willingness to pick this up, @andrew-s28! I agree with @VeckoTheGecko that your strategy makes sense; so please go ahead and propose some changes. Note that it doesn't need to be a fully functional PR; even a start would already be great. We're here to help :-)

@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Aug 16, 2024

@andrew-s28 just a note we've installed formatting across the codebase in the last couple days (#1655). I recommend merging with latest master soon to help minimise conflicts

@andrew-s28
Copy link
Contributor

Thanks for the heads up @VeckoTheGecko. Just synced my fork, so should be good to go! I'll keep an eye on it going forward, since it seems there's lots of great developments happening quickly.

PS thank you and @erikvansebille for all the work that is going into the project. I found the new projects tab really helpful for seeing where I could contribute (and the overall direction of the package development), so I think it is working well.

@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Aug 26, 2024

Related to this is the parameter netcdf_decode_warning used in the codebase.

netcdf_decodewarning : bool
            Whether to show a warning id there is a problem decoding the netcdf files.
            Default is True, but in some cases where these warnings are expected, it may be useful to silence them
            by setting netcdf_decodewarning=False.

No action required at the moment. I'll discuss and have a think about how we can deprecate this transparently with users. Let's create a warning class for it, and I can handle the code needed for deprecation

@andrew-s28
Copy link
Contributor

@VeckoTheGecko sounds good.

I should have a draft PR ready in the next couple of days, since I am assuming there will be some feedback in how I structure the new warnings classes and we can discuss from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants