EcohydrologyTeam / ClearWater-modules

A collection of water quality and vegetation process simulation modules designed to couple with water transport models.
MIT License
5 stars 1 forks source link

Algae Variables #28

Closed kewalak closed 1 year ago

kewalak commented 1 year ago

Tried setting up NSM1 like TSM. Need to double check but first pass added Algae Variables (constants/static, state, and dynamic), added Algae Processes and model file. Let me know if this generally looks okay. I think for the different module (algae, alkalinity, nitrogen, etc.) we would probably share the same variable files?

@xaviernogueira @imscw95

imscw95 commented 1 year ago

I agree. Seems like all NSM variables and processes should go into one file (one file for static, one for dynamic, one for state, and one for processes). For now I am making separate folders for the carbon, CBOD5, and alkalinity modules to help organize it for myself, but we can just paste them all together at the end. Maybe there's a reason to keep some of them separate but I am not seeing it.

Couple questions/comments based on looking at your code.

  1. I think the theta variable in the Arrhenius correction should be a static variable that can change based on user input. Right now it is hardcoded in but I think it can change.
  2. For reaction rates (e.g. kdp) that have not been temperature corrected yet, I was calling them kdp_20, since the correction is currently hardcoded to convert a rate from 20C to another temperature.
  3. My understanding was that all non-state variables that are computed by a 'process' should be considered dynamic. Looks like some of your static variables are associated with a process. Maybe I am misunderstanding how they should be split up. @xaviernogueira
xaviernogueira commented 1 year ago

@kewalak Oh and @imscw95 is right about this

  1. My understanding was that all non-state variables that are computed by a 'process' should be considered dynamic. Looks like some of your static variables are associated with a process. Maybe I am misunderstanding how they should be split up. @xaviernogueira

Static variables are defined as being set once on initialization. Therefore none should have a process associated with them, if they do they are dynamic.

Btw we will be renaming "dynamic" to "intermediate" for clarity, but don't worry about that yet I will do a find-and-replace.

kewalak commented 1 year ago

I believe I have addressed all of your comments in the most recent push.

imscw95 commented 1 year ago

@kewalak in the carbon modules, some processes rely on water depth in the cell. I was setting equal to a dynamic variable = to volume/surface area, which are both state variables pulled from the hydrodynamic model. I see that the algae module uses overall depth for each cell to calculate light penetration, which is a little different. I think we should make the overall depth a state variable that we can pull from the main hydrodynamic model.

kewalak commented 1 year ago

@imscw95 That sounds great to me, I will look into updating the code.

imscw95 commented 1 year ago

@kewalak in the carbon modules, some processes rely on water depth in the cell. I was setting equal to a dynamic variable = to volume/surface area, which are both state variables pulled from the hydrodynamic model. I see that the algae module uses overall depth for each cell to calculate light penetration, which is a little different. I think we should make the overall depth a state variable that we can pull from the main hydrodynamic model.

@kewalak I think we can just set depth = to volume/surface area, which are both pulled from the model. I made this comment thinking there were multiple vertical cell layers, which is not true!

kewalak commented 1 year ago

@imscw95 @xaviernogueira I restructured the variable files a little based on how I had the dictionaries set up before. Let me know what you think.

Also should I be merging this branch into main? The most recent push is sort of incomplete it was just where I got to yesterday evening.

xaviernogueira commented 1 year ago

@kewalak I'll look at it now! And yes I'd say this can go to main since this is a private repo. We can keep the issue open as work continues.

xaviernogueira commented 1 year ago

Looking now. Seems like all my comments were added. Feel free to merge and rebranch if you and @imscw95 will be overlapping code. If not there's no issue with just continuing with the branch.

FYI I'm on vacation now until Thursday