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

Icon namelist non optional #109

Merged
merged 8 commits into from
Jan 30, 2025
Merged

Icon namelist non optional #109

merged 8 commits into from
Jan 30, 2025

Conversation

leclairm
Copy link
Contributor

@leclairm leclairm commented Jan 27, 2025

As advised by @DropD , we remove the necessity of ICON namelists being apparently optional for ICON tasks by using @dataclass(kw_only=True). This is now used all over the code base.

Some doctests are also added to ConfigNamelist and ConfigIconTask

@leclairm leclairm force-pushed the ICON_namelist_non_optional branch from a684e91 to e4045ef Compare January 27, 2025 15:02
strangely not caught by ruff
also make validator idempotent even if unused yet
@leclairm leclairm requested review from agoscinski and DropD January 27, 2025 20:01
Copy link
Collaborator

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

I am fine with using the kw_only, it only restrains us from using the constructor with positional arguments which I don't think is important. Can we add a test that tests that it is optional? The test I see still adds a namelist.

EDIT: Wait as far as I experience, the namelist was not optional before. I thought this PR would make it optional, but the title says it is made non optional. If one does not have to modify anything in the namelist, then one does not need to specify the namelist in the config yaml as the names are hardcoded and they are assumed to be in the rootdir.

@leclairm
Copy link
Contributor Author

leclairm commented Jan 28, 2025

EDIT: Wait as far as I experience, the namelist was not optional before. I thought this PR would make it optional, but the title says it is made non optional. If one does not have to modify anything in the namelist, then one does not need to specify the namelist in the config yaml as the names are hardcoded and they are assumed to be in the rootdir.

We actually need the namelists (plural) input from the user. Otherwise we need to make assumptions on where they lives (so rootdir and no subfolder of it for instance) and on the file name, which is is only hard coded for the master namelist but not the other ones (there will always be at least 2 namelists). Before the PR, namelists was non optional in practice but optional in the class definition of ConfigIconTask. So this PR can be considered part of #88.

@agoscinski
Copy link
Collaborator

agoscinski commented Jan 30, 2025

Okay I see that it simplifies handling for now, and before this PR we only had it optional on the parsing and not optional on the workflow side. To make this consistent would be nice. But in principle we could make assumptions about where the namelists are and make it optional in the future?

on where they lives (so rootdir and no subfolder of it for instance)

We already make the assumption that they are relative to the rootdir, can't we make the assumption that they are in rootdir if not specified?

and on the file name, which is is only hard coded for the master namelist but not the other ones (there will always be at least 2 namelists).

But aren't the other namelists specified in the master namelist?

@leclairm
Copy link
Contributor Author

Frankly, I really don't see the point of making config namelists optional while being actually implicitly required

  • Pros:
    • [user] spare one specification in the config file
  • Cons:
    • [user] non explicit mention of the namelist files
    • [dev] adding all the code to treat the default case
    • [dev] handle the | None part of the type everywhere or add a canonical class just for that
    • [dev] test the default case specifically

@leclairm leclairm merged commit 2e28ea4 into main Jan 30, 2025
6 checks passed
@leclairm leclairm deleted the ICON_namelist_non_optional branch January 30, 2025 12:57
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