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

Refactoring (part 1) #18

Merged
merged 123 commits into from
Sep 5, 2023
Merged

Refactoring (part 1) #18

merged 123 commits into from
Sep 5, 2023

Conversation

mjaehn
Copy link
Contributor

@mjaehn mjaehn commented Jun 15, 2023

Updating and consolidating Processing Chain for ICON-ART (C2SM Task WG2 in 2023/1)

With the increasing number of supported model configurations and simulation options, parts of the processing chain (especially the main routine run_chain.py) have become rather complex and somewhat convoluted.

The following parts of refactoring the code are addressed in this PR:

  • change naming: target -> model, subtarget -> variant
  • provide input files as dictionary (later to be replaced by config.yaml file)
  • introduce models.yaml file for global model configurations
  • reduce size of input data for test cases
  • job scripts: target models directly by their name
  • run_chain.py: distinguish features and variants instead of models
  • Remove non-test cases
  • add example test case for icon-art
  • add example test case for icon-art-global
  • update README.md

This is part 1 of the refactoring task. Part 2 will mainly consist of the following:

  • Change case configuration files to a more "config-like" format: config.py -> config.yaml
  • Docs: Revision, updating, publish to github.io
  • Add icon-art-oem-ensemble test case
  • Check if variants and features can be unified
  • Further quality-of-life changes for more intuitive handling of the chain

@mjaehn
Copy link
Contributor Author

mjaehn commented Jul 26, 2023

launch jenkins

1 similar comment
@mjaehn
Copy link
Contributor Author

mjaehn commented Jul 27, 2023

launch jenkins

@mjaehn
Copy link
Contributor Author

mjaehn commented Jul 27, 2023

launch jenkins

1 similar comment
@mjaehn
Copy link
Contributor Author

mjaehn commented Jul 27, 2023

launch jenkins

@mjaehn mjaehn requested review from gredvis, efmkoene and jthanwer July 27, 2023 11:13
@mjaehn
Copy link
Contributor Author

mjaehn commented Jul 27, 2023

All tests are green. Ready to review 💡

Copy link
Contributor

@efmkoene efmkoene left a comment

Choose a reason for hiding this comment

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

Looks alright to me. Did I see correctly that there is no longer an ensembles testcase?

README.md Show resolved Hide resolved
# Nudging (meteorological and tracers)
era5_global_nudging = False
species_global_nudging = False
species2nudge = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there future plans of doing something with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jthanwer What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does species2nudge have any impact on the results or is this is only a feature to be implemented in the future?

cases/icon-art-oem-test/config.py Show resolved Hide resolved
Comment on lines +75 to +79
'oem_gridded_emissions_nc': ['tno_3cat.nc', 'OEM'],
'oem_vertical_profiles_nc': ['vertical_profiles.nc', 'OEM'],
'oem_hourofday_nc': ['hourofday.nc', 'OEM'],
'oem_dayofweek_nc': ['dayofweek.nc', 'OEM'],
'oem_monthofyear_nc': ['monthofyear.nc', 'OEM'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct in seeing that this specification for OEM differs between COSMO and ICON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but it could be adapted for COSMO to have it more uniform.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I have to say, it's actually quite neat. I'd support for COSMO following a similar quickfire settings procedure. If @gredvis agrees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree that it would be nice to have the same look-and-feel for COSMO

config/models.yaml Show resolved Hide resolved
jobs/emissions.py Outdated Show resolved Hide resolved
jobs/tools/write_cosmo_input_ghg.py Outdated Show resolved Hide resolved
@mjaehn
Copy link
Contributor Author

mjaehn commented Aug 2, 2023

@efmkoene Thanks for your review. Indeed, the ensembles cases is not included here, but it will come back as a test case in the next iteration (see description of this PR). Let me know if you are happy with the changes.

@mjaehn
Copy link
Contributor Author

mjaehn commented Aug 2, 2023

launch jenkins

@efmkoene
Copy link
Contributor

efmkoene commented Aug 2, 2023

I'm happy with the changes. I'll let @gredvis and @jthanwer answer the outstanding questions/clarifications; either or both can then approve the PR.

@mjaehn
Copy link
Contributor Author

mjaehn commented Aug 14, 2023

To move forward, I would like to merge this PR this week. @gredvis @jthanwer What are your opinions on the open points?

@gredvis
Copy link
Contributor

gredvis commented Aug 15, 2023

I briefly had a look at it together with Erik. Looks all good to me. I propose to move forward with the merge.

@gredvis gredvis closed this Aug 15, 2023
@gredvis
Copy link
Contributor

gredvis commented Aug 15, 2023

Sorry, I should not have closed the pull request

@gredvis gredvis reopened this Aug 15, 2023
Copy link
Contributor

@gredvis gredvis left a comment

Choose a reason for hiding this comment

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

Looks all good. Thanks for changing OAE to OEM at all places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants