Skip to content

Conversation

@AntonioTorga
Copy link

Basically that. Separated all the clases from the driver into model, observation, pair and analysis files. Each file contains just one class.

Changed all the import statements from relative to absolute, to prevent errors later when moving files around the repository.

Changed the class names to meet the PEP8 standard.

I already tested it with a big control file, but would really like for someone else to test it with other data. Mainly because i've been having some issues with external dependencies (netCDF4 and dask).

Copy link
Collaborator

@blychs blychs left a comment

Choose a reason for hiding this comment

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

Hi Antonio,
Thanks for doing this. I like the breaking up of the driver, which since now driver is a directory shouldn't really change much. I also like you going for absolute rather than relative imports.

My only worry is with the change of class names. It is true that PEP8 asks for CamelCase in the case of classes, but wouldn't this break our current scripts, since they all call of an = driver.analysis() ? If that is the case, I think we should either add the lowercase names as an alias or wait for v2 to do this, since v1.1 shouldn't have breaking changes.
Maybe you already did this, and I didn't see it.
Cheers
Pablo

@AntonioTorga
Copy link
Author

Oh yeah! You are right that would break scripts.
I could:
1- Export them from the package with an alias.
2- Undo the changes for now.

I think the second one would be safer, can't think of a case where the first one fails but that doesn't mean it doesn't exist. So i'll add it to this pull request.

@blychs
Copy link
Collaborator

blychs commented Dec 24, 2025 via email

@AntonioTorga
Copy link
Author

This is ready.
Changed back the class names to lowercase and ran all the tests. Everything seems to work as intended.

@rschwant rschwant requested a review from blychs January 12, 2026 17:37
Copy link
Collaborator

@blychs blychs left a comment

Choose a reason for hiding this comment

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

Just a few (very minor) suggestions. Let me know and I'll be happy to approve it

@AntonioTorga
Copy link
Author

Just a few (very minor) suggestions. Let me know and I'll be happy to approve it
Yeah! i think inside packages it's not that necessary but i agree that for consistency it would be better to have it as absolute.

Althought it has to be something like:
from melodies_monet.driver.module import class
Instead of
from melodies_monet.module import class

Already fixed and tested. Worked well.

Copy link
Collaborator

@blychs blychs left a comment

Choose a reason for hiding this comment

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

Great job, @AntonioTorga !!!

@rschwant
Copy link
Collaborator

Great job @AntonioTorga, do you want to resolve the conflicts? Perhaps something got merged in after you did this?

@AntonioTorga
Copy link
Author

Hi Becky! Merge conflicts are now solved and tested :)

@rschwant rschwant self-requested a review January 15, 2026 16:43
@rschwant
Copy link
Collaborator

I looked through the code and re-ran the doc examples.

@AntonioTorga I just found one small problem above. If we can update this, then I can re-run the AirNow CAM-Chem SE example (which is how I found this problem) and double check it now works and then I can approve and merge this in tomorrow.

@AntonioTorga
Copy link
Author

@rschwant Done! sorry for the delay. Let me know if you find any other errors.
Would be good to have more extensive checks so we can catch things like this earlier

Copy link
Collaborator

@rschwant rschwant left a comment

Choose a reason for hiding this comment

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

Great thanks, this now passes all the examples and agreed we need to make all the ReadTheDocs examples test cases too so we find problems faster and I don’t have to re-run them and manually check all the time. Hopefully we can do this soon.

@rschwant rschwant merged commit d8b97f4 into NCAR:develop Jan 20, 2026
1 check 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.

3 participants