Refactor MB06 #51
Refactor MB06 #51
Conversation
Codecov Report
@@ Coverage Diff @@
## sco_dev #51 +/- ##
===========================================
+ Coverage 74.54% 81.67% +7.12%
===========================================
Files 5 5
Lines 275 251 -24
===========================================
Hits 205 205
+ Misses 70 46 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
pydomcfg/utils.py
Outdated
|
|
||
| # TODO: | ||
| # Warning or error? | ||
| warnings.warn( |
There was a problem hiding this comment.
I personally would put an error: from personal experience, usually it is sufficient tiincrease the tolerance of a couple of orders, an many tines it is relating with the precision of your data ... in this way we force the user to be aware and maybe make the input more consistent precision wise
There was a problem hiding this comment.
OK, changed to error! Maybe we should decrease the default number of iterations... I'd get pretty mad if I wait hours to get an error and no return :)
Anyways, not urgent.
pydomcfg/utils.py
Outdated
| rmax: float, | ||
| tol: float = 1.0e-8, | ||
| max_iter: int = 10_000, | ||
| ) -> DataArray: | ||
| """ | ||
| This is NEMO implementation of the direct iterative method |
There was a problem hiding this comment.
I would add something like: This is the xarray version of NEMO implementation ....
There was a problem hiding this comment.
Reworded, although I didn't explicitly mention xarray as we use it pretty much everywhere and it should be clear from the docs. Also I removed the units, any units should work fine.
There was a problem hiding this comment.
Yes, I agree with your point ... it was just to make clear to the users (an us :) ) that this is only a more efficient version of the NEMO MB06 implementation
|
Very clear implementation, thanks @malmans2 ! |
Thanks! BTW, eventually I think we should make this function public. But no need to do it right now, we can wait until we're ready to merge |
|
All set! Not sure if you're ready to integrate this in your |
@oceandie if you are done with MB06, I think this can be merged into #33.
Needs a good check, but I think it's consistent with the implementation with loops.
I used this notebook to test.