llc rearray update

[Mikejmnez]

This is an update to llc_rearrange.py. It now uses exclusively xarray to manipulate datasets and remove faces (all or a subset) as a dimension (before bit of xarray and numpy, which was slow).

Some more changes will come in the next couple of weeks. But this is usable now.

Could use more testing. I will add more in the follow up PR.

[malmans2]

I think the test failure is unrelated to this PR. I didn't have the time to look into it, but tests started failing last week in the master branch as well. https://github.com/hainegroup/oceanspy/actions/runs/1806606996

[ThomasHaine]

Looks good to me...apart from the failing tests. Can we merge now or do the failing tests need to be fixed first?

[Mikejmnez]

I think keeping track of why these test are failing is important, but there is a way to go through with this PR. The issue seems to be two failing tests associated test_anim_Hsection (lines 85-101 in test_animate.py). One thing we could try is to comment those tests and go through the PR and fix this issue later... It is going to decrease coverage but it will only be temporal.

[ThomasHaine]

@malmans2 What do you prefer? Fix the failing tests first then merge or vice versa?

[codecov]

Codecov Report

Merging #214 (8470b7d) into master (0e19a23) will increase coverage by 1.30%.
The diff coverage is 89.75%.

❗ Current head 8470b7d differs from pull request most recent head 303f783. Consider uploading reports for the commit 303f783 to get more accurate results

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   95.30%   96.60%   +1.30%     
==========================================
  Files          10       10              
  Lines        3809     3569     -240     
  Branches      849      766      -83     
==========================================
- Hits         3630     3448     -182     
+ Misses         99       68      -31     
+ Partials       80       53      -27     

Flag	Coverage Δ
unittests	96.60% <89.75%> (+1.30%)	⬆️

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

Impacted Files	Coverage Δ
oceanspy/_ospy_utils.py	98.19% <50.00%> (+0.02%)	⬆️
oceanspy/utils.py	96.72% <66.66%> (ø)
oceanspy/_oceandataset.py	97.58% <87.50%> (+0.01%)	⬆️
oceanspy/llc_rearrange.py	87.59% <90.22%> (+4.66%)	⬆️
oceanspy/subsample.py	97.97% <100.00%> (ø)
oceanspy/animate.py	96.02% <0.00%> (-1.99%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e19a23...303f783. Read the comment docs.

[ThomasHaine]

Huh, now the tests pass...

[Mikejmnez]

Huh, now the tests pass...

@ThomasHaine the issue is still there, I commented the two failing tests in test_animate.py. I don't know what the issue is yet, but I guess I am suggesting that if we are OK with moving forward with this PR before fixing the issue, for now, commenting two tests allows this PR to pass. I describe what the failing tests are in PR #212

[ThomasHaine]

Huh, now the tests pass...

@ThomasHaine the issue is still there, I commented the two failing tests in test_animate.py. I don't know what the issue is yet, but I guess I am suggesting that if we are OK with moving forward with this PR before fixing the issue, for now, commenting two tests allows this PR to pass. I describe what the failing tests are in PR #212

Understood. I'm happy to merge.

[Mikejmnez]

Lets hear Mattia @malmans2 's opinion. This is clearly only a temporary solution. I just don't really have time to figure what the issue with the update in PR #212 is.

[malmans2]

Hi!
I'm quite busy in the next couple of weeks and I don't have much time to look at it. Feel free to merge when you're ready (maybe Ali could take a quick look and approve?).

Regarding tests, don't worry about those, they are not related to this PR. But I don't think commenting out the tests is the best way to go. I'd merge this PR even if tests are failing, then we'll address those issues in a separate PR.

Are you able to merge even if tests are failing? Let me know if you can't and I'll change your permissions, I think you are "maintainer" right now.

[Mikejmnez]

Got it. I didn't know you could approve with failing tests. I un-commented the failing tests (unrelated with this PR) and will wait for @asiddi24 to take a look at this PR.

I don't think I can merge when tests are failing..

[malmans2]

You are admin now... can you merge? You should be able to see this:

[Mikejmnez]

You are admin now... can you merge? You should be able to see this:

Yes , I can now. wow, with great power comes great responsibility...