Skip to content

Dwd derived#1615

Open
mspils wants to merge 5 commits intomainfrom
dwd_derived
Open

Dwd derived#1615
mspils wants to merge 5 commits intomainfrom
dwd_derived

Conversation

@mspils
Copy link
Collaborator

@mspils mspils commented Feb 23, 2026

@mspils mspils mentioned this pull request Feb 23, 2026
Copy link
Member

@gutzbenj gutzbenj left a comment

Choose a reason for hiding this comment

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

Dear @mspils ,

thanks for working on this!

Just two comments:

  • there's some commented out code, could you remove it?
  • could there be more "speaking" names for the parameters?

There's also also a test

def test_metadata_parameter_names(parameter_names: list[str], metadata: dict) -> None:

that checks that parameter names are part of the one big enum at
that unifies parameter names across services. Could the parameters fit in there?

Comment on lines 656 to 663
pl.col("station_id"), # .str.strip_chars(" ").str.pad_start(5, "0"), #.cast(str)
pl.col("start_date"), # .str.to_datetime("%Y%m%d", time_zone="UTC",strict=False),
pl.col("end_date"), # .str.to_datetime("%Y%m%d", time_zone="UTC",strict=False),
pl.col("height"), # .str.strip_chars().cast(pl.Float64),
pl.col("latitude"), # .str.strip_chars().cast(pl.Float64),
pl.col("longitude"), # .str.strip_chars().cast(pl.Float64),
"name",
"state",
Copy link
Member

Choose a reason for hiding this comment

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

Please remove commented out code here.

@mspils
Copy link
Collaborator Author

mspils commented Mar 2, 2026

Just to clarify:
I should change the name of the parameters to the names in parameter.py if there is a fitting one.
If there is no fitting one I add a new one?
That seems to be the case for at least:

  • radiation_global_uncertainty
  • sunshine_duration_uncertainty
  • TSLS05 (soil temperature, but below a specific type of ground)
  • TSSL05 (soil temperature, but below a specific type of ground)

Currently there DWDDerivedMetaData is also not part of test_metadata_parameter_names. I guess I should change that as well?

@gutzbenj
Copy link
Member

gutzbenj commented Mar 2, 2026

@mspils yes, exactly, the idea is to get unified names across the different services. The soil temperature names mostly come from the NOAA metadata. I hope the Derived parameters fit in there! If you need further assistance please let me know!

@mspils
Copy link
Collaborator Author

mspils commented Mar 4, 2026

The categories of soil/cover types used by the DWD are different from the NOAA, so I had to add new Parameters.

I decided to not add separate Parameters for the daily/monthly soil Parameters in most cases, since they are aggregated in the way one would expect:
Evatransporation -> Sum
Soil Moisture -> Mean

Exception to this is Frost/Thaw depth which I added as max.
I also added the other derived parameters by @jb-at-bdr , they may have some input on the new variable_names for heating_degree/cooling_degree. I tried to make it more consistent with ECC/NOAA.

@gutzbenj I assume documentation is my responsibility? What is your stance on LLM generated stuff? Of course manually checked for correctness.

@gutzbenj
Copy link
Member

gutzbenj commented Mar 5, 2026

That sounds great, I'll check the naming!

Please go ahead, any documentation is better then no documentation!

Update:
Names lgtm.

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