Skip to content

allows user defined defaults for stmp and spfh#146

Open
benjaminleighton wants to merge 1 commit into
mainfrom
feat_custom_defaults_spfh_stmp
Open

allows user defined defaults for stmp and spfh#146
benjaminleighton wants to merge 1 commit into
mainfrom
feat_custom_defaults_spfh_stmp

Conversation

@benjaminleighton

Copy link
Copy Markdown
Collaborator

@VHernaman I think this implements the defaults we patched into our older winds to sflux allowing a user to customize defaults for spfh or stmp

@hot007 hot007 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.

I believe this change should have no overall effect on what the code produces, but it would be good to test calling it with and without providing values for stmp_var and spfh_var. I think the only change should be to add the metadata description that the missing value default is in place for (either of) these two variables.

Comment thread rompy/schism/data.py
if variable == "spfh_name":
missing = 0.01
missing = self.spfh_default
elif variable == "stmp_name":

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.

If we're setting stmp values to -999 when missing, as we would for any other variable, then this elif is not needed as it's effectively captured by the else. It's therefore also unnecessary to define stmp_default on lines 164-167 I believe.

Comment thread rompy/schism/data.py
-999,
description="default value for surface air temperature when variable is missing",
)
spfh_default: float = Field(

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.

I think just adding this one is sufficient, indeed the only difference this should make to the code behaviour is adding the metadata for what this missing value is doing - which is nice, but overall no net change I think.
Check - is the code producing appropriate forcing files when these two variables are or are not supplied when calling it?

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