hainegroup / oceanspy

A Python package to facilitate ocean model data analysis and visualization.
https://oceanspy.readthedocs.io
MIT License
96 stars 32 forks source link

decouple mooring_array from cutout + bug + refactor + ... #399

Closed Mikejmnez closed 8 months ago

Mikejmnez commented 8 months ago

Closes #378 #379 #384

Checklist

Changes This is essentially Part 2 of work I began back in March, when I started to work on mooring_array to allow compatibility with xoak. Here, a substantially more challenging and overwhelming effort which I began in July, decouples mooring_array from cutout, and addresses various open issues. In particular it closes: #378 , #379, #384 . See also #398.

To enumerate the features / fixes:

  1. Allows mooring_array to be calculated without going through cutout first, whenever the extra argument serial=True is passed.
  2. serial=False is the default behavior, and the new code implementation is separate from the previous way mooring_array works. This allows legacy code to remain intact.
  3. Unpins scipy, and as a result OceanSpy will fail to build a tree when there is nan-data in the coordinates. This can happen with cutout + llc-data at high lats, resulting in an Error.
  4. Because of 3, mooring_array + serial=True should be preferably used when face is a dimension.
  5. mooring_array and also returns the pair (diffX, diffY), which is necessary for mooring_volume_transport. These variables are otherwise computed in mooring_volume_transport in a way that is incompatible with datasets with face as a dimension.
  6. mooring_array + serial=True, computes the coordinates [XU,YU, XV, YV] from the coordinates of the mooring_array. Before, these were computed during cutout for the entire area encompassing the mooring_array, which was extremely inefficient. These 4 variables are required to be present in the dataset when computing mooring_volume_transport.
  7. Significantly improves coverage.

In addition, this PR also refactors quite a bit subsample, although I had to create a bunch of functions and added them to llc_rearrange.

To see some of the behavior, I uploaded a testing notebook to GH, that is rather clean. The link is here: https://github.com/Mikejmnez/Notebooks/blob/main/ospy_mooring_latest.ipynb

Also, see this notebook for a comparison between the behavior of mooring_array via cutout (serial=False) and the new implementation (serial=True) with ECCO, one single snapshot. https://github.com/Mikejmnez/Notebooks/blob/main/Mooring_volume_transport.ipynb

Mikejmnez commented 8 months ago

I need to incorporate the new pre-comit behavior from prevous PR.

codecov[bot] commented 8 months ago

Codecov Report

Merging #399 (a956724) into main (15bbb74) will increase coverage by 2.63%. The diff coverage is 94.32%.

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   94.15%   96.79%   +2.63%     
==========================================
  Files           9        9              
  Lines        4194     4987     +793     
  Branches     1004     1189     +185     
==========================================
+ Hits         3949     4827     +878     
+ Misses        154       90      -64     
+ Partials       91       70      -21     
Flag Coverage Δ
unittests 96.79% <94.32%> (+2.63%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
oceanspy/utils.py 97.93% <100.00%> (+11.08%) :arrow_up:
oceanspy/subsample.py 98.11% <98.31%> (+5.48%) :arrow_up:
oceanspy/compute.py 98.54% <57.14%> (-0.28%) :arrow_down:
oceanspy/llc_rearrange.py 93.62% <93.82%> (+8.84%) :arrow_up:

... and 1 file with indirect coverage changes

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Mikejmnez commented 8 months ago

Yeah, pre-commit has changed preferred behaviors (see #385 ) since I started this branch back in July 6th. Incorporating those is a one-liner.

Like I said, I will be adding some additional tests to improve coverage today, but I needed to get this PR started even though it wasn't 100% ready b/c it has been a massive effort that I can't wait be done with...

Mikejmnez commented 8 months ago

hmm I though adding shapely into the ci/environment.yaml would be enough...

Mikejmnez commented 8 months ago

hmm. I had to add shapely as a dependency in pyproject.toml.

Mikejmnez commented 8 months ago

this is taking way to long... I am going to restart the build of 3.11

Mikejmnez commented 8 months ago

Good, finally. FYI test_opening_and_saving within oceanspy/tests/test_open_oceandataset.py often times will get stuck, that is what was happening here, and it often happens in my local computer.

Mikejmnez commented 8 months ago

Feel free to merge this. I introduced some functions in llc_rearrage to be used with mooring_array/stations that I think can and should be used in the arctic transformation, i.e. they'd greatly improve the behavior of the transformation. And so both llc_rearrage and its testing needs refactoring just like @MaceKuailv says. But I don't think I have the capacity for this now...