-
Notifications
You must be signed in to change notification settings - Fork 18
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
refactoring of harmonic model code to apply as ufunc on xarray #206
refactoring of harmonic model code to apply as ufunc on xarray #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - thanks! I left some initial comments. Can you:
- address those comments (see below)
- remove the empty
untitled
file - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice not to hard-code the dimension name here. See e.g.
mesmer/mesmer/core/smoothing.py
Line 7 in bdec598
def lowess(data, dim, *, frac, use_coords_as_x=False, it=0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah O.K. will keep this in mind
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - some first comments.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.