-
Notifications
You must be signed in to change notification settings - Fork 2
Phosphorus, N2, and Pathogens and a very incomplete SedFlux #61
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
Conversation
Still a work in progress
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 38.58% 39.36% +0.77%
==========================================
Files 33 33
Lines 1161 1138 -23
==========================================
Hits 448 448
+ Misses 713 690 -23 ☔ View full report in Codecov by Sentry. |
xaviernogueira
left a comment
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.
Left a few comments. A handful of typehints in _n2.py need to be updated. Also there are some non-.py files that are showing up weird. Should be a quick fix!
Other than that great work, everything looks amazing, and all tests are passing so this is near-merge ready.
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.
The pricess should have xr.DataArray or np.ndarray as in input/output typehints (as opposed to "float"), try a find and replace?
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.
This is an old file (along with all the other _XX.py files) that I kept around just since we were still translating everything over.
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.
Is this meant to be a .py file?
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.
Is this meant to be a .py file?
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.
Is this meant to be a .py file? Also if you want to keep scrap files, i'd either not commit them or use commented out code where the purpose of keeping the old/scrap code around it made clear.
imscw95
left a comment
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 great.
Phosphorus, N2, and Pathogens should generally be good to go. Sedflux still needs a good amount of work.