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

Specx workflow and NCL receipt #39

Merged
merged 31 commits into from
Sep 20, 2024
Merged

Specx workflow and NCL receipt #39

merged 31 commits into from
Sep 20, 2024

Conversation

jukent
Copy link
Collaborator

@jukent jukent commented May 17, 2024

PR Summary

Demonstrates how to use the scipy.signal module for spectral analysis and a 1-to-1 NCL receipt.

Related Tickets & Documents

Closes NCAR/geocat-comp#124

PR Checklist

General

  • PR includes a summary of changes
  • Link relevant issues, make one if none exist

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

github-actions bot commented May 17, 2024

Meowdy! See your PR preview:
🔍 Git commit SHA: c652a4f
✅ Deployment Preview URL: https://NCAR.github.io/geocat-applications/_preview/39

@jukent
Copy link
Collaborator Author

jukent commented Jun 4, 2024

I might be missing a couple places where I have to list these notebooks in order for them to visible. Do we have documentation on that yet?

@jukent jukent requested a review from anissa111 June 4, 2024 17:20
@anissa111
Copy link
Member

I might be missing a couple places where I have to list these notebooks in order for them to visible. Do we have documentation on that yet?

Ahh, yeah. So ultimately I want to automate a lot of that, but for now, some of it is in the contributors guide.

@jukent jukent requested review from kafitzgerald and cyschneck June 24, 2024 20:05
applications/data_analysis/data_analysis.rst Outdated Show resolved Hide resolved
ncl/ncl_index/ncl_index.rst Outdated Show resolved Hide resolved
ncl/ncl_index/ncl-index-table.csv Outdated Show resolved Hide resolved
@cyschneck
Copy link
Contributor

There are two main notebooks here: one that lives under data_analysis (data_analysis/spectral_analysis.ipynb) and the receipt and notebook that live as NCL files (ncl/NCL_spectral_analysis.ipynb and ncl/receipts/NCL_receipt_spectral_alaysis)

So spectral analysis should show up on the sidebar in two places, @anissa111 correct? One under Applications > Data Analysis and the other under NCL Applications

@jukent
Copy link
Collaborator Author

jukent commented Jun 24, 2024

Still unclear on the repetition of entries in NCL_index and NCL_applications

@jukent jukent requested a review from cyschneck June 24, 2024 21:42
@anissa111
Copy link
Member

Still unclear on the repetition of entries in NCL_index and NCL_applications

Right, I think I'm also confused about what's going on here.

There are two main notebooks here: one that lives under data_analysis (data_analysis/spectral_analysis.ipynb) and the receipt and notebook that live as NCL files (ncl/NCL_spectral_analysis.ipynb and ncl/receipts/NCL_receipt_spectral_alaysis)

So spectral analysis should show up on the sidebar in two places, @anissa111 correct? One under Applications > Data Analysis and the other under NCL Applications

@cyschneck, are you asking if the same physical notebook should be linked in two places (no, in my opinion) or are you asking if spectral analysis content should be in multiple different categories?

@cyschneck
Copy link
Contributor

@anissa111, because there are two separate notebooks, they should both appear separately on the side bar? So I am asking are you asking if spectral analysis content should be in multiple different categories? Since this covers both a Python applications notebook and an NCL to Python notebook

@jukent jukent requested a review from cyschneck June 26, 2024 14:54
ncl/ncl_entries/specx_anal.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/specxy_anal.ipynb Outdated Show resolved Hide resolved
@anissa111
Copy link
Member

Getting back to this, I think it would be worth doing some higher-level updates before getting into some more fine-grained feedback.

I know it's been a minute since this has seen attention, so here are the things I noticed need updated from team discussions about since last time.

  • Remove NCL references from python content (applications/data_analysis/spectral_analysis.ipynb)
  • Take a pass through plotting/viz sections for non-essential lines (like plt.tight_layout(), probably plt.show(), possibly figsize=(...) if it doesn't effect the readability)
  • Consider combining ncl/ncl_entries/specx_anal.ipynb and ncl/ncl_entries/specxy_anal.ipynb NCL entries and possibly reconfigure to include NCL content from the python content/receipt and to be more in line with the NCL template and other NCL entries.
  • Make the receipt (ncl/receipts/specx_anal.ipynb) more consistent with receipt template and other reciepts (in the way of less narrative content, more testing, etc)

@anissa111
Copy link
Member

Ah, gosh. I meant for that to show up as a commented review, my bad. I'm going to remove myself as a requested reviewer so that I can be re-requested/notified when it's is ready for another look.

@anissa111 anissa111 removed their request for review August 7, 2024 20:52
@jukent jukent requested review from cyschneck and anissa111 August 13, 2024 23:17
@jukent
Copy link
Collaborator Author

jukent commented Aug 13, 2024

figsize() and tightlayout() does affect readability on the subplots.

Copy link
Member

@anissa111 anissa111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm seeing some more of those mathjax things here:
image

Copy link
Collaborator

@kafitzgerald kafitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is looking really good.

Some more general comments/questions (more specific comments inline):

  • It looks like the outputs are still stored stored on this though - I think the plan is to clear those now that there's infra to run the notebooks.
  • It looks like specxy_anal is in the NCL Index, but not the receipts. I assume that's an item for later, but wanted to double check.

ncl/ncl_entries/ncl_entries.rst Outdated Show resolved Hide resolved
ncl/ncl_index/ncl-index-table.csv Outdated Show resolved Hide resolved
ncl/ncl_index/ncl-index-table.csv Outdated Show resolved Hide resolved
ncl/receipts/specx_anal.ipynb Outdated Show resolved Hide resolved
ncl/receipts/specx_anal.ipynb Outdated Show resolved Hide resolved
applications/data_analysis/spectral_analysis.ipynb Outdated Show resolved Hide resolved
applications/data_analysis/spectral_analysis.ipynb Outdated Show resolved Hide resolved
applications/data_analysis/spectral_analysis.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/specx_specxy_anal.ipynb Outdated Show resolved Hide resolved
@jukent jukent merged commit af55e01 into NCAR:main Sep 20, 2024
2 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 20, 2024
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.

Implement NCL's spectra functionality: specx_anal, specx_ci and specxy_anal
4 participants