OceanParcels / Parcels

Main code for Parcels (Probably A Really Computationally Efficient Lagrangian Simulator)
https://www.oceanparcels.org
MIT License
298 stars 138 forks source link

Parcels cleanup #1620

Open VeckoTheGecko opened 4 months ago

VeckoTheGecko commented 4 months ago

Rather than create many issues regarding code cleanup, let's use this issue to collate cleanup related items. Feel free to edit this description to add more items.


https://github.com/OceanParcels/parcels/blob/df473012bfca9ea5001063d2293a3157282ac5d2/parcels/kernel.py#L125-L132


Details

Some of the codebase imports individual functions from standard library packages. For example: https://github.com/OceanParcels/parcels/blob/804ef58846a8de88147fda31c00bed387a341e0f/parcels/kernel.py#L1-L13 This is quite confusing as it can become unclear where functions like `parse` come from (reading `ast.parse()` is instantly recognisable as parsing the abstract syntax tree, while `parse()` hints to a parcels specific function with no added context). This is unclear and clogs up the namespace. This should be changed especially for places where it could lead to confusion.


Details

The parcels error codes defined within `tools/statuscodes.py` largely inherit from `RuntimeError` resulting in a flat error hierarchy. Whether an error is a parcels error is handled separately via a dictionary: https://github.com/OceanParcels/parcels/blob/b0fb29fde98f9e9ee891f472bb1ee3e6c992683a/parcels/tools/statuscodes.py#L114-L119 Along with code along the lines of: ``` except tuple(AllParcelsErrorCodes.keys()) as error: ``` Deciding a new hierarchy (e.g., by subclassing from a `ParcelsError` class instead of `Runtime`), would clean things up to a simple `except ParcelsError as e:` in the code. Its important that such a hierarchy is sound on a conceptual level for the package. The dictionary `AllParcelsErrorCodes` can still be generated, or perhaps the status codes can live in the class itself with `StatusCode` pulling from it.


We currently have autoformatting of python code via the use of the black autoformatter. This is run on all Jupyter notebooks, and the example scripts in the doc website. It would be good to gradually expand this use to the whole codebase as refactoring is done.

Rather than use an include list for black, we should configure (once #1609 is merged) an ignore list on a file by file basis in pyproject.toml. As files reach compliance with black, they can be removed from the ignore list. I prefer doing it gradually with consideration to readability, as opposed to doing it unilaterally in one swoop on the whole codebase. Some lines are really long in the codebase and black may make them unreadable if no refactoring is also done.


Multiple parts of the codebase have code duplication when handling input parameter checking (typechecking and value checking). Surely there's a better way to do this. Take inspiration from existing popular open source packages.

E.g.,

https://github.com/OceanParcels/parcels/blob/be5e757ab2972c3b08361d43edbe2824cef546be/parcels/particleset.py#L884-L885

https://github.com/OceanParcels/parcels/blob/be5e757ab2972c3b08361d43edbe2824cef546be/parcels/kernel.py#L565-L566


Details

Enable pyupgrade in the Ruff linting config. Currently some strings in the codebase use Python 2 string formatting, and Pyupgrade can't automatically upgrade these. Once these strings (which are flagged by Ruff) are fixed, we can enable pre-commit. Look at the data types being formatted into the string, and ensure that the updated format string is functionally the same as the one before (some of the strings are in the code generator which get compiled to C code, so this is particularly important).


Details

_A plugin for flake8 finding likely bugs and design problems in your program. Contains warnings that don't belong in pyflakes and pycodestyle_ -[flake8 bugbear repo](https://github.com/PyCQA/flake8-bugbear) Make sure to choose which codes to ignore and which not to. 100% compliance is not the goal.


Details

https://github.com/OceanParcels/Parcels/blob/2a876f827ac954bd2fdefab96c52064caa889588/parcels/kernel.py#L483-L489




erikvansebille commented 4 months ago

Thanks for flagging this, @VeckoTheGecko. I quickly browsed through the code, but don't know of any other pre-v2 or even pre-v3 code snippets.