cryotools / cosipy

Coupled snowpack and ice surface energy and mass balance model in Python
GNU General Public License v3.0
51 stars 29 forks source link

Fix test suite and various bugs #54

Closed gampnico closed 8 months ago

gampnico commented 1 year ago

Summary

The checksum for this branch's model output is identical to the main branch, so there are no changes in model output/accuracy. There are no breaking changes.

Details

The newest versions of xarray do not support Python 3.8. The requirements file now produces a minimum working environment without gdal, and selects the appropriate distributed version. It is not possible to install postprocessing support with pip for Python 3.11 as richdem is missing.

To install a fully working environment use conda install --file dev_requirements.txt (not pip). This includes postprocessing support and all optional dependencies. For Python 3.9/3.10 you may safely upgrade to the latest version of distributed. If in doubt, downgrade distributed to 2021.3 and 2022.3. For Python 3.11, distributed should be pinned to 2023.2 as later versions are broken by tornado.

I've fixed the broken test suite - it now runs with a simple pytest . and supports coverage, test serialisation etc. Duplicate tests and objects are now fixtures. Tests are comprehensive, serialised, and split into chunks, which should make it easier to triage errors. .coveragerc and .gitignore are more robust.

A warning is now raised to stdout when percolation fails its sanity check and outputs more meltwater than is available - the sanity check was passing silently. The timestep is included.

I've fixed the non-functional field_plots and vtk_plots and refactored duplicate code. VTK plots no longer consist of red smudges and both modules are more user-friendly.

Some instances of namespace pollution were increasing overhead and leading to clashes. I've refactored these, but it comes at a small increase in compilation time.

I've added type hinting and updated older docstrings with missing/incorrect parameters, keeping the docstring format consistent within modules. I've kept typos in variable names for backwards compatibility with older output files.

gampnico commented 11 months ago

Additional changes

Summary

Details

Printing a warning when percolation fails its sanity check is really expensive as numba doesn't support warnings. I would recommend disabling it entirely when profiling the model - comment out the last line in cosipy.modules.percolation.percolation().

np.int and np.float are deprecated since numpy v1.20. Their documentation recommends using either builtins or a sized alias. I replaced them with builtins.

I removed unused imports and refactored wildcard imports that were only being used to import a single class. I refactored more duplicate code, and some cases where the same (constant) value was recalculated multiple times instead of being stored as a variable. Conditionals now use implicit False or interval comparisons where possible.

Some QoL changes affect some outdated docstrings, type hints, and formatted strings. Other changes are purely cosmetic: spelling, inconsistent indentation, mixed tabs/spaces, random whitespace. I avoided any kind of general linting/PEP8 because I think it'll be a nightmare for others to merge downstream - there's simply too much.

As before, the checksum for this branch's model output is identical to the main branch, and there are no breaking changes.