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

Update TEM configuration #279

Merged
merged 9 commits into from
Mar 5, 2024

Conversation

justin-richling
Copy link
Collaborator

It seems like it might make more sense to move the TEM configuration into the case subsections as requested by @cecilehannay.

This will move each TEM configuration into case and baseline sections of config file and will update the TEM files to account for the configuration change.

Move each TEM configuration into case and baseline sections of config file
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks good, but did have some questions and a few change requests.

scripts/averaging/create_TEM_files.py Outdated Show resolved Hide resolved
scripts/averaging/create_TEM_files.py Outdated Show resolved Hide resolved
scripts/averaging/create_TEM_files.py Show resolved Hide resolved
scripts/averaging/create_TEM_files.py Outdated Show resolved Hide resolved
scripts/averaging/create_TEM_files.py Outdated Show resolved Hide resolved
scripts/averaging/create_TEM_files.py Outdated Show resolved Hide resolved
scripts/plotting/tem.py Outdated Show resolved Hide resolved
scripts/plotting/tem.py Show resolved Hide resolved
scripts/plotting/tem.py Outdated Show resolved Hide resolved
scripts/plotting/tem.py Show resolved Hide resolved
@justin-richling
Copy link
Collaborator Author

justin-richling commented Jan 30, 2024

@nusbaume I think I addressed your comments/suggestions. Thanks for looking this over, the changes are in when you get a chance to review it again. Thanks!

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Super close! Just one last question.

scripts/averaging/create_TEM_files.py Outdated Show resolved Hide resolved
@justin-richling
Copy link
Collaborator Author

@nusbaume There was also a bug for the TEM plots if the file exists and redo plot is false, it created an empty plot and saved that to the web.

I've tested this with model vs model and model vs obs. If you don't mind just looking at these two changes again that'd be great.

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Everything looks good to me now. Thanks @justin-richling!

@justin-richling justin-richling merged commit b961522 into NCAR:main Mar 5, 2024
7 checks passed
@justin-richling justin-richling deleted the tem-config-update branch March 5, 2024 22:09
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