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

Clean up code base, add pre-commit, add dependencies #7

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Aug 31, 2023

Addresses issues raised in #2, but does not close it.

Ping @agstephens @sol1105

Changes

  • Adds a pre-commit configuration for automated code linting
  • NetCDF grid downloader has been cleaned up for readability (Note: Cleaning performed with the aid of ChatGPT)
  • Base Python has been changed to Python3.9 (follows from NEP-0029)

Work needed

  1. There are a few instances where CLISOPS is called. If this is to become a dependency of CLISOPS, we need to be implementing the functions required here to prevent a circular dependency loop.
  2. The reference grid files are very large! If we only require the grids themselves, it's preferable that the variable values as well as the time dimensions be removed from all files. At the current size, this package cannot be hosted with its data on PyPI.

Proposal

Would this library better serve as a module of CLISOPS instead? Are there instances where the tools here would be used independently of CLISOPS? Given the tight focus and coupling with CLISOPS, this seems more like a specialized helper library. If so, we can very easily push this directly into CLISOPS.

@Zeitsperre Zeitsperre added enhancement New feature or request help wanted Extra attention is needed labels Aug 31, 2023
@Zeitsperre Zeitsperre self-assigned this Aug 31, 2023
Copy link
Collaborator

@sol1105 sol1105 left a comment

Choose a reason for hiding this comment

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

This looks great, thank you.
I will deal with the #FIXME points once this PR is merged.

@@ -2,6 +2,9 @@
from pathlib import Path

import xarray as xr

# FIXME: This is a circular dependency, as clisops will be listing roocs-grids as a dependency
Copy link
Collaborator

Choose a reason for hiding this comment

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

The scripts directory is basically a directory with auxiliary scripts to prepare the grid files. None of its contents are imported by the roocs_grids module. I don't know if there is a proper approach for something like this. Maybe one could rename it to aux and move it one directory up, next to tests, roocs_grids so it won't get accidentally imported?

@@ -30,6 +33,7 @@
# Create clore.Grid object
g = clore.Grid(ds)
# Drop variables and global attributes
# FIXME: This is broken in newer versions of xarray
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will need to have a look in the clisops regridding PR when time allows, to solve this.

[[ ! -e "$targetpath"/grid_T63_lsm_binary.nc ]] && {
mv "$targetpath"/sftlf_fx_MPI-ESM1-2-LR_1pctCO2_r1i1p1f1_gn.nc "$targetpath"/grid_T63_lsm_binary.nc
ncks -A -h -v areacella $targetpath/areacella_fx_MPI-ESM1-2-LR_1pctCO2_r1i1p1f1_gn.nc "$targetpath"/grid_T63_lsm_binary.nc
nccopy -d 9 -s "$targetpath"/grid_T63_lsm_binary.nc "$targetpath"/grid_T63_lsm_binary_def.nc
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will need to extend the compression to all downloaded grid files and remove unnecessary attributes, variables and dimensions. By just applying nccopy -d 9 -s to all the grid files, the volume could be shrinked from 45M to 2.4M, which would be fine? The volume was one of the reasons, roocs_grids was a separate repository and not part of the clisops regridding PR.

I will follow up your PR with another one dealing with the compression and further cleaning of the netcdf files.

@Zeitsperre Zeitsperre marked this pull request as ready for review September 5, 2023 18:33
@Zeitsperre
Copy link
Collaborator Author

@sol1105

Your comments sound good. I'll merge this now and leave you to work on the comments raised here.

All the best!

@Zeitsperre Zeitsperre merged commit c47ec43 into main Sep 5, 2023
@Zeitsperre Zeitsperre deleted the cleanup branch September 12, 2023 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants