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

Consider time-dependent processes #79

Open
sjordan29 opened this issue Mar 29, 2024 · 5 comments
Open

Consider time-dependent processes #79

sjordan29 opened this issue Mar 29, 2024 · 5 comments
Labels
nsm Specific to NSM module tsm Specific to TSM module

Comments

@sjordan29
Copy link
Contributor

sjordan29 commented Mar 29, 2024

Considering both tsm and nsm, look through variables and processes that are time-dependent and consider how the (user-defined) time dim will be used to make sure the timestep is correctly handled.

As part of this issue, we should rename the timestep variable that's used in processes to dt to avoid confusion with the timestep incrementor.

@sjordan29 sjordan29 added tsm Specific to TSM module nsm Specific to NSM module labels Mar 29, 2024
@sjordan29
Copy link
Contributor Author

Example processes that depend on time:

For TSM - these seem to assume a timestep of 1 second: https://github.com/EcohydrologyTeam/ClearWater-modules/blob/main/src/clearwater_modules/tsm/processes.py

  • dTdt_sediment_c
  • dTdt_water_c

For NSM, time dependent processes have a timestep variable, which is, by default, 1.
https://github.com/EcohydrologyTeam/ClearWater-modules/blob/main/src/clearwater_modules/nsm1/processes.py
Examples include:

  • Ap
  • Ab

@aufdenkampe aufdenkampe added this to the NSM Initial Release milestone May 14, 2024
@sjordan29
Copy link
Contributor Author

@imscw95 @kewalak @aufdenkampe, I'm going to work on resolving this dt issue. @imscw95, I know you dug into this a little bit in the HEC-RAS "module tester," so it'd be great to get your insight.

NSM: Looking at the state variables and time-dependent processes (e.g., Ap) in NSM, it looks like it's currently set with units equal to 1 day and passing all tests.

TSM:

  • q_sediment converts units for thermal diffusivity from m2/day to m2/second.
  • dTdt_water_c -- working out these units I end up with Kelvin/second (?), so that further confirms the timestep is in units of seconds.
    • As an aside, should cp_water be in J/kg/C instead of J/kg/K? I'm a little confused on the units here

Seems like we'll want to align dt across modules to default to the same thing (either 1 day or 1 second). Any preferences? Should we skip aligning the modules for now and just focus on getting dt as an input for TSM and not worry about aligning modules?

Thanks!

@imscw95
Copy link
Contributor

imscw95 commented Jul 26, 2024

Hey @sjordan29. It'll definitely be easier to convert the TSM units from seconds to days just thinking about the amount of variables/coefficients we'd need to convert. Also, I'd think modeling these processes at the seconds scale is overkill. Days is probably too coarse for temperature though cause of daily cycles, so if we want them aligned at a useful timescale, we'd probably need to change the NSM units, which would be a major undertaking. Is having them on different time scales feasible? I think hours is the ideal timescale for temp and we could probably convert that pretty easily.

On the cp_water, is the temperature unit relevant, since the amount of energy to change 1 degree C vs 1 degree K would be the same right, since the two are always different by a constant value? I think the cp_water curve, which relates water temp to the cp_water value would be sensitive to the water temp unit. I did some googling and the cp_water estimates were more or less the same for both J/gC and J/gK.

@sjordan29
Copy link
Contributor Author

Thanks! Sounds good, I think I will work on addressing this in TSM. Since TSM is passing all tests against calculated values -- can you confirm that the comparison was done at 1 second output? We can keep the values the same even with this change, and just modify the dt value. But I want to make sure that I understand what we're comparing it against before making that change!

@imscw95
Copy link
Contributor

imscw95 commented Aug 13, 2024

Yes, confirmed that the TSM tests were for dt = 1 second.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nsm Specific to NSM module tsm Specific to TSM module
Projects
None yet
Development

No branches or pull requests

3 participants