Skip to content

Conversation

@thomas-pijls
Copy link
Contributor

@thomas-pijls thomas-pijls commented Dec 9, 2025

What was done

Use backgroundwatertemperature instead of air_temperature_in_cell to compute heat flux out of the water in the excess model

Added a testcase to test this behaviour

Renamedjatem to temperature_model and parameterised its options (TEMPERATURE_MODEL_NONE=0, TEMPERATURE_MODEL_TRANSPORT=1, TEMPERATURE_MODEL_EXCESS=3, TEMPERATURE_MODEL_COMPOSITE=5)

Renamed set_temperature_models function to update_meteo_forcing

Evidence of the work done

  • Clear from the issue description

Tests

  • Added testcase e02_f020_c050_cooling_to_backgroundwatertemperature

Documentation

  • Updated backgroundwatertemperature keyword description in User Manual to reflect its use in the heat flux computations in the excess model

Issue link

@adrimourits
Copy link
Contributor

Please have a look at Wilbert's comment (12 Dec) in the issue; he prefers an error instead of a warning.

@thomas-pijls
Copy link
Contributor Author

Erik just replied to Wilbert's comment that he will make a separate issue to turn the warning into an error; so it's outside the scope of this issue. Do you agree?

@adrimourits
Copy link
Contributor

Please add a subfolder "doc", both to testcase
"dsc-testbench/cases/e02_dflowfm/f006_external_forcing/c176_freezing_with_background_salinity"
and
"dsc-testbench/cases/e02_dflowfm/f020_heat_flux/c050_cooling_to_backgroundwatertemperature"

This is needed because a script collects all docs from all testcases and it fails (on purpose) if a doc folder is missing.

You can start by copying a "doc" folder from another testcase, most probably it will not contain relevant information. Please spent an hour to put all relevant information that you have at hand in the doc latex file.

@adrimourits
Copy link
Contributor

Erik just replied to Wilbert's comment that he will make a separate issue to turn the warning into an error; so it's outside the scope of this issue. Do you agree?

I had a chat with Erik: he will add a comment to the issue: can you please change the warning into an error as part of this issue?

Can you ask me to review again when you changed this and when you added doc folders to the testcases?

Copy link
Contributor

@adrimourits adrimourits left a comment

Choose a reason for hiding this comment

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

I'm surprised that besides solving the review-comments, you also did a global search/replace on jatim-> temperature_model (which is a big improvement). Did you communicate this upfront with somebody? Instead of 2 files changed, your PR now involves 42 files changed.

If the pipeline is green, you can merge it.

Copy link
Contributor

@adrimourits adrimourits left a comment

Choose a reason for hiding this comment

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

Sorry, another comment

end subroutine set_external_forcings

!> set_temperature_models
subroutine set_temperature_models(time_in_seconds)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry :-(, I have another comment:
Please rename subroutine update_meteo_forcing into set_temperature_model (without the s at the end).

update_meteo_forcing is wrong because:

  • update suggests data is being updated (but it's just flags/ec-module preparation stuff)
  • meteo suggest also wind and precipitation (but it's just the temperature exchange model)

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