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

climatology Timeavg remake #373

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

climatology Timeavg remake #373

wants to merge 35 commits into from

Conversation

kiihne-noaa
Copy link
Contributor

@kiihne-noaa kiihne-noaa commented Feb 28, 2025

Describe your changes

timeavg fucntion can now accept multiple files. Tests work on two files, but any number should work. Creates a new, combined netcdf file, then runs options with it. Deletes file after output is created. Extra parameters added for fre-python-tools scenario to keep it running as it was before the change. Added versions of original tests that function with multiple files.

Checklist before requesting a review

  • [ Y] I ran my code
  • [ Y] I tried to make my code readable
  • [ Y] I tried to comment my code
  • [ Y] I wrote a new test, if applicable
  • [ N] I wrote new instructions/documentation, if applicable
  • [ Y] I ran pytest and inspected it's output
  • [ Y] I ran pylint and attempted to implement some of it's feedback
  • No print statements; all user-facing info uses logging module

@mlee03
Copy link

mlee03 commented Feb 28, 2025

@kiihne-noaa, could you change the title to start with "CLIMATOLOGY"? Thank you!

@kiihne-noaa kiihne-noaa marked this pull request as ready for review March 13, 2025 18:30
@ilaflott ilaflott self-requested a review March 14, 2025 16:44
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.

Can you go through the checklist that other PRs use? The template can be copied from here. It will help you self-review, and then i can do a final-review.

An example of a thing to consider: do we really need an entire two_file_test.py, that looks largely like a carbon copy of test_generate_time_averages.py? The similarity suggests that perhaps the new two-file test cases should be a set of tests under test_generate_time_averages.py, which would reduce code-repeat, which makes it easier to keep the code up-to-date (fewer files to update).

It's good work- just needs polish.

@Kiihne
Copy link

Kiihne commented Mar 14, 2025

Can you go through the checklist that other PRs use? The template can be copied from here. It will help you self-review, and then i can do a final-review.

An example of a thing to consider: do we really need an entire two_file_test.py, that looks largely like a carbon copy of test_generate_time_averages.py? The similarity suggests that perhaps the new two-file test cases should be a set of tests under test_generate_time_averages.py, which would reduce code-repeat, which makes it easier to keep the code up-to-date (fewer files to update).

It's good work- just needs polish.

Will do on the checklist. For the tests, I had separated them intially for simplicity, and then kept them separate at the end due to file length. trying to balance number of files and not having too long files. Combining them would cause the single test file to be over 450 lines long. Just confirming before I do that?

@kiihne-noaa kiihne-noaa changed the title Timeavg remake climatology Timeavg remake Mar 14, 2025
@ilaflott
Copy link
Member

ilaflott commented Mar 14, 2025

Can you go through the checklist that other PRs use? The template can be copied from here. It will help you self-review, and then i can do a final-review.
An example of a thing to consider: do we really need an entire two_file_test.py, that looks largely like a carbon copy of test_generate_time_averages.py? The similarity suggests that perhaps the new two-file test cases should be a set of tests under test_generate_time_averages.py, which would reduce code-repeat, which makes it easier to keep the code up-to-date (fewer files to update).
It's good work- just needs polish.

Will do on the checklist. For the tests, I had separated them intially for simplicity, and then kept them separate at the end due to file length. trying to balance number of files and not having too long files. Combining them would cause the single test file to be over 450 lines long. Just confirming before I do that?

I say go for it, part of the reason its so long is because of the commented out test cases- there's obviously going to be room for improvement in the future there.

For now though- all you should have to do is basically migrate over the references to your new test files and call them something else variable name wise, then move over the test cases and rename them smth like test_cdo_time_avg_two_files

Copy link
Collaborator

@ceblanton ceblanton left a comment

Choose a reason for hiding this comment

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

Let's avoid binary Netcdf files in the repository as they are difficult to remove later, and not trackable by git.

Smallest-possible .cdl files (created by ncgen)

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.

5 participants