Skip to content

Type fix: tuple to list in DynamicalCoreConfig#90

Merged
jjuyeonkim merged 1 commit into
NOAA-GFDL:developfrom
jjuyeonkim:20251022_small_type_fix
Oct 24, 2025
Merged

Type fix: tuple to list in DynamicalCoreConfig#90
jjuyeonkim merged 1 commit into
NOAA-GFDL:developfrom
jjuyeonkim:20251022_small_type_fix

Conversation

@jjuyeonkim

Copy link
Copy Markdown
Collaborator

Description
A type fix in DynamicalCoreConfig.from_f90nml().

Without this fix, there are failures in Pace linting checks.

How Has This Been Tested?
CI tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • Targeted model if this changed was triggered by a model need/shortcoming

@jjuyeonkim jjuyeonkim marked this pull request as ready for review October 22, 2025 17:55

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks okay to me. An alternative I see is to change DEFAULT_DYCORE_NML_GROUPS to be a list instead of a tuple. That would also avoid the need to copy it into a list. Your choice.

And if you want to be sure the type error doesn't come back in the future, you could add a simple test case, creating a dycore config from_f90nml(), similar to what exists in test_config_from_yaml.py

@romanc

romanc commented Oct 23, 2025

Copy link
Copy Markdown
Collaborator

or - and I just though about that - if tuple is a more or equally correct type, you could

  • assuming is one is more correct than the other: temporarily allow both, list and tuple, in f90nml_as_dict(), then removing the other after you change all usage to the more correct type
  • assuming both are equally correct: use Sequence[str] (or an explicit list[str] | tuple[str]) as type in f90naml_as_dict()

those two also avoid the copy from one type to another (which isn't really necessary since all that is done is iterating over all elements, which can be done for list and tuple).

@jjuyeonkim

Copy link
Copy Markdown
Collaborator Author

I think that modifying f90nml_as_dict() to use Sequence[str] instead of list[str] is better than my solution in this PR.

However, there are a lot of changes going on in NDSL at the moment, so I'm going to try to push this one for now to get the pyshield namelist changes in. Then, I'll circle back to make the change afterwards.

@jjuyeonkim jjuyeonkim added this pull request to the merge queue Oct 24, 2025
Merged via the queue into NOAA-GFDL:develop with commit a0c972d Oct 24, 2025
4 of 6 checks passed
@jjuyeonkim jjuyeonkim deleted the 20251022_small_type_fix branch December 23, 2025 14:59
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