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

Implement warnings in Parcels #1672

Merged
merged 17 commits into from
Sep 2, 2024

Conversation

andrew-s28
Copy link
Contributor

@andrew-s28 andrew-s28 commented Aug 28, 2024

Resolves #1600 by implementing the warnings module in Parcels.

This is presented as a draft PR since I think some of the details of the implementation will benefit from discussion to make sure that things are in their best state.

Here, I have implemented three custom warning classes, FieldSetWarning, KernelWarning, and FileWarning that I think encompass the majority of warnings that Parcels was raising. A few warnings (e.g., ParticleSet is empty...) did not fit neatly into these categories, so I assigned them RuntimeWarning.

I also added these warnings to the documentation, with a short description of each that would hopefully be a resource for users trying to address the warning. The naming of warnings and their docstrings is something that could probably benefit from the input of Parcels maintainers.

Tests were added for a few of the warnings (i.e., the test makes sure the warning is properly raised), but I couldn't get test cases working for all of the possible cases, since some are fairly complicated to raise (especially those that have to do with I/O or chunking, like the FileWarning). Input here is welcome also.

Warning Classes
Class Docstring
FieldSetWarning Warning that is raised when there are issues in the construction of the FieldSet or its Grid. These warnings are often caused by issues in the input data dimensions or options selected when loading data into a FieldSet.
FileWarning Warning that is raised when there are issues with input or output files. These warnings can be related to file chunking, naming, or decoding issues. Chunking issues in particular may negatively impact performance (see also https://docs.oceanparcels.org/en/latest/examples/documentation_MPI.html#Chunking-the-FieldSet-with-dask)>
KernelWarning Warning that is raised when there are issues with the Kernel. These warnings often result from issues in the FieldSet or user-defined Kernel that are passed into the Parcels Kernel loop.

It is worth noting that I was unable to completely remove the logger module, since Parcels uses it to print info (which is distinct from the warnings). See below for the list of where this logging remains. I left a bare-bones logging implementation in the logging.py module to support these. It is also worth noting that Parcels also implements custom exceptions in the statuscodes.py module.

Remaining Logging Info
File Code
fieldset.py logger.info(f"Generating FieldSet output with basename: {filename}")
kernel.py logger.info(f"Compiled {self.name} ==> {self.src_file}")
particleset.py logger.info(f"Output files are stored in {output_file.fname}.")
rng.py logger.info(f"Compiled ParcelsRandom ==> {self.src_file}")

I think it could make sense to bring the warnings, statuscode exceptions, and logger all under the same module (perhaps exceptions.py?) but that does potentially leave the StatusCode class in a weird place. I do think making this change would simplify code base. If we are in favor of making this change, then I can update the PR accordingly (or open a new one if it does not fit into the scope of this one).

@erikvansebille
Copy link
Member

Wow, thank you so much for this PR, @andrew-s28! I had a quick lok through it, and it's beautifully concise and seems pretty complete!

@VeckoTheGecko and I will go more carefully through it in the coming days; but this certainly is a fantastic starting point for (if not a complete version of) this refectoring!

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @andrew-s28 ! This is exactly the direction we're wanting to take the codebase.

Additional feedback:

Here, I have implemented three custom warning classes, FieldSetWarning, KernelWarning, and FileWarning that I think encompass the majority of warnings that Parcels was raising. A few warnings (e.g., ParticleSet is empty...) did not fit neatly into these categories, so I assigned them RuntimeWarning.

I like this structure of warnings.

It is worth noting that I was unable to completely remove the logger module

I think its worth keeping the logging module, and will be useful in future. Though its nice to have the .warning_once/.info_once stuff cleaned away.

I think it could make sense to bring the warnings, statuscode exceptions, and logger all under the same module (perhaps exceptions.py)

I agree with the note about structure, but this would be a breaking change for users that I'm not comfortable rolling out in a minor release. It would require an update to tutorials, and (as you mention) there is no "home" for StatusCodes. Let's keep with the current structure.

parcels/field.py Outdated Show resolved Hide resolved
parcels/field.py Show resolved Hide resolved
tests/test_tools.py Outdated Show resolved Hide resolved
parcels/field.py Outdated Show resolved Hide resolved
tests/test_tools.py Outdated Show resolved Hide resolved
@VeckoTheGecko VeckoTheGecko marked this pull request as ready for review August 29, 2024 12:14
@andrew-s28
Copy link
Contributor Author

Thanks for the feedback @VeckoTheGecko and @erikvansebille. I think I have incorporated most or all of your suggested changes with the recent commits.

I agree with the note about structure, but this would be a breaking change for users that I'm not comfortable rolling out in a minor release. It would require an update to tutorials, and (as you mention) there is no "home" for StatusCodes. Let's keep with the current structure.

I didn't really consider that this would be a breaking change, so this makes sense to me. Perhaps something to keep in mind for future major version releases then 😄.

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Very nice implementation, no further comments!

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

Looks good! Just waiting on ✅. Just merged with master and pushed some minor edits (e.g., .. deprecated docstring syntax not working with individual parameters, only on a function/object/module level)

This comment was marked as off-topic.

@VeckoTheGecko VeckoTheGecko merged commit 286558c into OceanParcels:master Sep 2, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Better control of warnings for users
3 participants