hainegroup / oceanspy

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

enhancing LLC rearrange #268

Closed Mikejmnez closed 2 years ago

Mikejmnez commented 2 years ago

This new PR improves the efficiency of llc_rearrange by allowing it to transform only the data relevant to the range of lats and long defined within subsample.cutout. That is, llc_rearrange takes the arguments

XRange = np.array, list or None.
YRange = np.array, list or None.

This is, llc_rearrange identifies the cutout region, and only transforms it returning a new dataset without complex topology. The older version of llc_rearrange would transform first all the data within the faces that contain the XRange and YRange values, and then drop most of it... See the example below, wherein the blue region in the map describes the cutout region when XRange and YRange (defined by A05 data). llcrearrange transforms the smallest rectangle containing such data, as opposed to first transforming the faces with "surviving data".

Screen Shot 2022-10-23 at 9 59 12 PM

I was going to create this pull request on Friday, but then I noticed a weird behavior when XRange and YRange define a region entirely in the Pacific Ocean. The cutout performs exactly as it should be, but the ordering of the indexes is reversed. I will open an issue explaining the behavior.

This PR also:

codecov[bot] commented 2 years ago

Codecov Report

Merging #268 (aaac862) into main (51d1cc5) will decrease coverage by 0.15%. The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
- Coverage   95.51%   95.36%   -0.16%     
==========================================
  Files          10       10              
  Lines        3636     3842     +206     
  Branches      766      811      +45     
==========================================
+ Hits         3473     3664     +191     
- Misses        111      114       +3     
- Partials       52       64      +12     
Flag Coverage Δ
unittests 95.36% <88.88%> (-0.16%) :arrow_down:

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

Impacted Files Coverage Δ
oceanspy/_oceandataset.py 97.36% <50.00%> (ø)
oceanspy/llc_rearrange.py 88.76% <86.34%> (+1.16%) :arrow_up:
oceanspy/subsample.py 98.22% <95.65%> (+0.22%) :arrow_up:
oceanspy/utils.py 83.41% <100.00%> (+4.83%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Mikejmnez commented 2 years ago

Well this is weird... All tests are passing.

MaceKuailv commented 2 years ago

Am I hallucinating? I saw the same fail as mine and the bots on this pull request last night.

Mikejmnez commented 2 years ago

I checked on the Files changed for this PR, and it states that I am supposedly changing .github/workflows/ci.yaml and .pre-commit-config.yaml but I am not, relative to hainegroup/oceanspy/. You can take a look at the code on the main repository. What I did is I did change those relative to my own fork (Mikejmnez/oceanspy) since I started working on this fork before those changes happened and needed to keep on track of the changes...

So I don't understand why all tests are passing, but that is great news I guess!

ThomasHaine commented 2 years ago

Interesting. Could be a github bug? Anyway, feel free to merge!

malmans2 commented 2 years ago

Hi! I noticed this discussion glancing through. Keep in mind that the github action that installs the environment is cached. I think the way it's set up right now, the cache is invalidated if the environment.yml changes or once a day. Each PR is cached separately. Not sure if it explains what's happening, but here is the link to the GH action in case you want to have a look and/or adjust the settings we have right now: https://github.com/mamba-org/provision-with-micromamba