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

Parcels cleanup #1620

Open
8 of 11 tasks
VeckoTheGecko opened this issue Jul 26, 2024 · 1 comment
Open
8 of 11 tasks

Parcels cleanup #1620

VeckoTheGecko opened this issue Jul 26, 2024 · 1 comment
Labels
cleanup Cleaning up legacy code coding/Python

Comments

@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Jul 26, 2024

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.

  • Remove v2 backward compatability
Details

Some code in the codebase has been introduced to maintain backwards compatibility with pre-v2 code. The mentions I've found (by Ctrl+shift+F for 'v2' through .py files) were the following:

https://github.com/OceanParcels/parcels/blob/035159a1b4002bc1550cebd275f93c0adaa8513d/parcels/field.py#L483-L484

https://github.com/OceanParcels/parcels/blob/2910326d6882033f0b28f8eb74484bd09e40b271/parcels/kernel.py#L207-L208

Removing this code and any knock-on code would be good.

@erikvansebille please comment below if you know other relevant areas of the codebase.


  • Replace fix_indentation with function call from textwrap
Details

Replace with function call from the standard library `textwrap`.

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


  • Change imports of standard library to module imports and not function imports
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.


  • Improve parcels error codes (determine hierarchy)[requires internal discussion]
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.


  • [ONGOING] Expanding black compliance
Details

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.


  • Reduce code duplication of value checking
Details

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


  • Enable pyupgrade on ruff linting
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).


  • Enable bugbear on ruff linting
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

Make sure to choose which codes to ignore and which not to. 100% compliance is not the goal.


  • Remove erroneous calls to os.path in cleanup_remove_files() and clean up function
Details

Parcels/parcels/kernel.py

Lines 483 to 489 in 2a876f8

@staticmethod
def cleanup_remove_files(lib_file, all_files_array, delete_cfiles):
if lib_file is not None:
if os.path.isfile(lib_file): # and delete_cfiles
[os.remove(s) for s in [lib_file, ] if os.path is not None and os.path.exists(s)]
if delete_cfiles and len(all_files_array) > 0:
[os.remove(s) for s in all_files_array if os.path is not None and os.path.exists(s)]


  • Remove self.static_srcs declaration from kernel.py (isn't used in the codebase it seems)

  • Reduce code duplication in fieldset setup in tests (e..g, test_partialslip_nearland_zonal, test_partialslip_nearland_meridional). Consolidate into functions (or fixtures in conftest.py) as needed

@VeckoTheGecko VeckoTheGecko converted this from a draft issue Jul 26, 2024
@VeckoTheGecko VeckoTheGecko added cleanup Cleaning up legacy code help wanted good first issue Good for new parcels developers labels Jul 26, 2024
@erikvansebille
Copy link
Member

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.

@VeckoTheGecko VeckoTheGecko changed the title Remove v2 backward compatability Parcels cleanup Jul 29, 2024
@VeckoTheGecko VeckoTheGecko pinned this issue Jul 29, 2024
VeckoTheGecko added a commit that referenced this issue Aug 5, 2024
@VeckoTheGecko VeckoTheGecko removed help wanted good first issue Good for new parcels developers labels Oct 23, 2024
VeckoTheGecko added a commit that referenced this issue Oct 23, 2024
contributes to #1620
VeckoTheGecko added a commit that referenced this issue Oct 23, 2024
VeckoTheGecko added a commit that referenced this issue Oct 23, 2024
contributes to #1620
VeckoTheGecko added a commit that referenced this issue Oct 23, 2024
VeckoTheGecko added a commit that referenced this issue Oct 28, 2024
contributes to #1620
VeckoTheGecko added a commit that referenced this issue Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleaning up legacy code coding/Python
Projects
Status: Backlog
Development

No branches or pull requests

2 participants