-
Notifications
You must be signed in to change notification settings - Fork 45
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
Docs #168
base: master
Are you sure you want to change the base?
Docs #168
Conversation
removed some unused things
some other things to fix, but would probably want to do so in xarray first, since mostly copied from there
I think this is what it meant at least
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Thanks for these edits @zmoon ! :) |
@zmoon Can you run black on |
@roxyboy, when are you planning to make the Also would you mind if I switch from |
"Default value of lag was zero (centered output coordinates) " | ||
"and is now set to transformed coordinate's attribute 'direct_lag', " | ||
"defaulting to zero if that attribute is not set." | ||
) | ||
warnings.warn(msg, FutureWarning) |
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.
Should this still be FutureWarning
given that it has already been changed?
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 defaults is still False
for these flags I think in the current version.
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 default lag
is still None
, but the behavior has changed slightly (ca652c9), since it now tries the default_lag
attr before defaulting to 0.0
, whereas before pretty much nothing was done for lag=None
, making it effectively zero. So maybe just normal UserWarning
instead of FutureWarning
?
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.
Ok sure, I'm ok with UserWarning
Also how big is the dataset for the MITgcm example? Could it be included in the repo or somewhere else where it could be downloaded? |
the latitude coordinates are different by floating point rounding error, needed to replace before subtracting
Thanks so much for this much needed maintenance @zmoon! We really appreciate your efforts! 🏆 I am fine with changing any of the configurations in the docs to make things easier going forward. myst_nb is definitely fine.
We could put it in Zenodo and use Pooch. Or we could use one one of the Pangeo Cloud datasets - https://catalog.pangeo.io/browse/master/ocean/channel/ |
This is a good question. What's the normal period/cycles packages allow for a deprecation...? |
Great work! I've found some small typos in the notebooks of zmoon:docs. I cannot comment it directly here, because the diff is too large. What is the best-practice-procedure here? Should I fork zmoon:docs and pull a request into his repo? One more thing: What about the |
I wanted to correct the other two notebooks that haven't been refactored in this PR: #185. |
View / edit / reply to this conversation on ReviewNB KevinYanesG commented on 2022-04-28T14:53:14Z Now I learned that we can review the notebooks cell by cell with this tool. As I said, I think in the last sentence we should have |
Update docs, configure some things that were not yet configured, ...
Remaining:
*
to other signatures