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

Add analysis tools #273

Merged
merged 25 commits into from
Dec 16, 2024
Merged

Add analysis tools #273

merged 25 commits into from
Dec 16, 2024

Conversation

menzel-gfdl
Copy link
Contributor

@menzel-gfdl menzel-gfdl commented Nov 22, 2024

Describe your changes

Add some analysis tools to:

  • install analysis scripts
  • run analysis scripts

Issue ticket number and link (if applicable)

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback

@menzel-gfdl menzel-gfdl changed the title Add analysis tools Draft: Add analysis tools Nov 22, 2024
@menzel-gfdl menzel-gfdl marked this pull request as draft November 25, 2024 16:15
@ilaflott ilaflott self-requested a review December 11, 2024 15:06
Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

Well aware this is still in draft mode, but this is not far off from being able to be merged at all. Excellent work!

This needs the new noaa-gfdl::analysis_scripts package req added to environment.yml. If that module is indended to be pure-python package, we should add it to setup.py as well.

Without this, a currently doc'd development approach involving conda env create -f environment.yml breaks, and thats why create_test_conda_env.yaml is likely failing.

After doing that and making sure the pipeline passes, merge the latest 100 or commits to main into this branch to make sure nothing clashes. I doubt any clashes will come up... but no need to speculate, we have a pipeline!

Thrilled to approve this when its ready

meta.yaml Show resolved Hide resolved
Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

This might be a lesson in why one never says, "it's almost done!" Apologies for bringing a pox-upon-the-house!

I'm pretty sure this is getting caught up on some of click and python's most classic pit falls. I think I caught most of them.

fre/analysis/freanalysis.py Outdated Show resolved Hide resolved
fre/analysis/freanalysis.py Show resolved Hide resolved
fre/analysis/subtools.py Show resolved Hide resolved
fre/analysis/tests/test_subtools.py Show resolved Hide resolved
fre/fre.py Show resolved Hide resolved
fre/tests/test_fre_analysis_cli.py Outdated Show resolved Hide resolved
fre/tests/test_fre_analysis_cli.py Outdated Show resolved Hide resolved
@menzel-gfdl menzel-gfdl marked this pull request as ready for review December 13, 2024 23:46
@menzel-gfdl menzel-gfdl changed the title Draft: Add analysis tools Add analysis tools Dec 13, 2024
Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

We note a few pylint nits that have no impact, and some venv error handling within analysis_scripts that could be improved with some thought.

I think we're ready to merge this and to start exploring what's possible with the new functionality. This is a great step in fre-cli's development. Thanks @menzel-gfdl and @ceblanton for the quality work!

@ilaflott ilaflott merged commit f6c8cdc into NOAA-GFDL:main Dec 16, 2024
2 checks passed
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.

2 participants