refactoring of harmonic model code to apply as ufunc on xarray #206
Conversation
There was a problem hiding this comment.
Nice - thanks! I left some initial comments. Can you:
- address those comments (see below)
- remove the empty
untitledfile - run the linting (code auto-formatting) tools over your code (you may have to install them first)
isort .black .flake8 .(you may have to fix some stuff for this to run cleanly)
- let me know how strict I should be with my review
To address my comments you can either commit them online ("add suggestion to batch" -> "commit suggestions") and then you need to git pull origin refactor_mesmer_m_harmonic_model so that you have the changes available locally. OR you can just address them manually on your laptop, create a new commit and then git push origin refactor_mesmer_m_harmonic_model.
Let me know if you need support!
mesmer_m/harmonic_model.py
Outdated
| fit_to_bic_np, | ||
| X, | ||
| Y, | ||
| input_core_dims=[["time"],["time"]], |
There was a problem hiding this comment.
It would be nice not to hard-code the dimension name here. See e.g.
mesmer/mesmer/core/smoothing.py
Line 7 in bdec598
There was a problem hiding this comment.
Ah O.K. will keep this in mind
snath-xoc
force-pushed
the
refactor_mesmer_m_harmonic_model
branch
from
79f91a0
to
85b3a5d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
==========================================
+ Coverage 87.90% 88.00% +0.09%
==========================================
Files 40 40
Lines 1745 1750 +5
==========================================
+ Hits 1534 1540 +6
+ Misses 211 210 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mesmer_m/harmonic_model.py
Outdated
|
|
||
| bic_score[i_n - 1] = calculate_bic(len(y), i_n, mse) | ||
|
|
||
| n_sel = np.argwhere(bic_score == bic_score.min())[0][0] + 1 |
There was a problem hiding this comment.
| n_sel = np.argwhere(bic_score == bic_score.min())[0][0] + 1 | |
| n_sel = np.argmin(bic_score) + 1 |
There was a problem hiding this comment.
I reverted my changes and opened #410. I think we can merge this now.
Thanks for the initial changes, I accepted them, I will get to the rest of your points on the list by Friday. As for strictness, as long as it's constructive I'm willing to learn (I'm definitely a beginner right now so no worries). I may need to read up over the linting stuff as well.