Skip to content
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

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Docs #168

wants to merge 26 commits into from

Conversation

zmoon
Copy link
Contributor

@zmoon zmoon commented Nov 25, 2021

Update docs, configure some things that were not yet configured, ...

  • clean up Conda env
  • fix GH issue links in whats-new
  • RTD theme set in conf
  • add intersphinx/crossreferences
  • clean up docstrings
  • main API page organized; use top-level namespace for the generated documentation of the functions
  • edit other text a bit

Remaining:

  • example nbs
  • add * to other signatures

I think this is what it meant at least
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

xrft/xrft.py Show resolved Hide resolved
@roxyboy
Copy link
Member

roxyboy commented Nov 28, 2021

Thanks for these edits @zmoon ! :)

xrft/xrft.py Show resolved Hide resolved
@roxyboy
Copy link
Member

roxyboy commented Nov 28, 2021

@zmoon Can you run black on detrend.py? The pre-commit tests seems to be failing...

@zmoon
Copy link
Contributor Author

zmoon commented Nov 28, 2021

@roxyboy, when are you planning to make the true_* arguments True by default? Just wondering, as currently the examples generate warnings related to that.

Also would you mind if I switch from nbsphinx to myst_nb to render the nbs? nbsphinx is not doing well in the cases with inline code inside strong or links, e.g. in the main example. And with myst_nb, we can easily cross-reference API and use admonitions etc.

"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)
Copy link
Contributor Author

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?

Copy link
Member

@roxyboy roxyboy Dec 4, 2021

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.

Copy link
Contributor Author

@zmoon zmoon Dec 4, 2021

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?

Copy link
Member

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

@zmoon
Copy link
Contributor Author

zmoon commented Nov 28, 2021

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
@rabernat
Copy link
Collaborator

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.

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?

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/

@roxyboy
Copy link
Member

roxyboy commented Dec 4, 2021

@roxyboy, when are you planning to make the true_* arguments True by default? Just wondering, as currently the examples generate warnings related to that.

This is a good question. What's the normal period/cycles packages allow for a deprecation...?

@roxyboy roxyboy mentioned this pull request Dec 7, 2021
@zmoon zmoon mentioned this pull request Dec 13, 2021
@roxyboy roxyboy marked this pull request as ready for review January 18, 2022 09:16
@KevinYanesG
Copy link

KevinYanesG commented Apr 28, 2022

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?
image
image
--> I think these should be xrft.fft and xrft.ifft, respectively.

One more thing: What about the xrft.fft FutureWarnings, shouldn't they be triggered only when using xrft.dft?

@KevinYanesG
Copy link

I wanted to correct the other two notebooks that haven't been refactored in this PR: #185.

@review-notebook-app
Copy link

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 xrft.fft and xrft.ifft.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants