Ijm alk pom

[imscw95]

No description provided.

[codecov]

Codecov Report

Merging #47 (7d24a6e) into main (2c240d7) will decrease coverage by 0.04%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   35.47%   35.44%   -0.04%     
==========================================
  Files          33       33              
  Lines        1122     1123       +1     
==========================================
  Hits          398      398              
- Misses        724      725       +1     

Files	Coverage Δ
src/clearwater_modules/nsm1/state_variables.py	0.00% <0.00%> (ø)

[imscw95]

Not sure what the failed codecov means.

[xaviernogueira]

Not sure what the failed codecov means.

Don't worry about it now, it is telling you that the code you added is not tested in the automated pytest suite. I will make an Issue reflecting that and we can loop back to it once the whole module is closer to finalized.

Also, going to review your PR now.

[xaviernogueira]

Nice job! Mind explaining the similar functions at the top of POM/processes.py? Otherwise, it is great PR, no changes to request!

Time to merge unless @aufdenkampe wants to review it too?

[xaviernogueira]

Wondering how kdb_T() and kpom_T() differ?

[imscw95]

I think this is where my lack of understanding in the implementation of the processes is coming in. I thought they would require different processes cause they have different input variables, and the implementation of the processes are dependent on the names of the static variables that feed into them (such as in sorting the order of processes).

[xaviernogueira]

Ah! So I'm glad you said that now I understand where your confusion comes from exactly. So here is the thing, the arguments that the process functions take must match the name of a variable. BUT the name of the function does not matter, and any number of variables can share a process function (i.e., a case where two variables of different names are calculated the exact same way).

[xaviernogueira]

For example:

Variable(
  name='A',
  ...
  use='static',
)

def func_1(A: xr.DataArray) -> xr.DataArray:
  return A * 2

Variable(
  name='B',
  ...
  use='dynamic',
  process=func_1,
)

Variable(
  name='C',
  ...
  use='dynamic',
  process=func_1,
)

In this case dynamic variables B and C are calculated with the same func_1(), which takes variable A as an argument. I can change the name of func_1 arbitrarily, but the code won't work if I change the argument name For example:

VALID:

# note that "func_1" was changed to "not_func_1"
def not_func_1(A: xr.DataArray) -> xr.DataArray:
  return A * 2

Variable(
  name='B',
  ...
  use='dynamic',
  process=not_func_1,
)

Variable(
  name='C',
  ...
  use='dynamic',
  process=not_func_1,
)

INVALID:

# note that "A" was changed to "not_A"
def func_1(not_A: xr.DataArray) -> xr.DataArray:
  return not_A * 2

Variable(
  name='B',
  ...
  use='dynamic',
  process=func_1,
)

Variable(
  name='C',
  ...
  use='dynamic',
  process=func_1,
)

If I wanted the second (invalid) code update to work, I'd have to also change the name attribute of Variable A to:

Variable(
  name='not_A',
  ...
  use='static',
)

[xaviernogueira]

So in your case, if the arguments (inputs) of a process function match exactly real Variable names "via their Variable.name attribute", and the return value is the same, then only one function is needed and multiple Variables (with different names) can reference it.

Let me know if you understand!

[imscw95]

In this POM case, the arguments are different. We need the arrhenius correction to work for kdb_20 and kpom_20 (and for different theta values as well). I see you say that any number of variables can share a process function, but that seems like it conflicts with: "the arguments of said function should match the variable names that will be passed in."

If kdb_T and kpom_T are both associated with the same process, how would you set it up so that kdb_T is computed with the kdb_20 argument and kpom_T is computed with the kpom_20 argument?

[xaviernogueira]

Good use of xr.where, things look good here!