Phosphorus, N2, POM, and PX

[kewalak]

Still a few wrinkles to iron out. The following are test that are failing since they have negative values as test answers, and I added code so none of the constituents can have negative concentrations.

Carbon
test_change_kbod_20
test_change_kah_20_user
test_change_kaw_20_user

DOX
test_change_krb_20

CBOD
test_change_kbod_20
test_changed_ksbod_20
a75305c

[imscw95]

Only thing that seems weird is the default pressure of 2000+ mb. I think the range is like 900-1100.

[sjordan29]

Looks like the following tests failed:

• FAILED tests/test_10_nsm_carbon_calculations.py::test_changed_kbod_20 - assert 0.8048187759642682 ± 8.0e-03 == -45.77
• FAILED tests/test_10_nsm_carbon_calculations.py::test_changed_kah_20_user - assert 0.0 ± 1.0e-12 == -0.16
• FAILED tests/test_10_nsm_carbon_calculations.py::test_changed_kaw_20_user - assert 0.0 ± 1.0e-12 == -0.0063
• FAILED tests/test_11_nsm_DOX_calculations.py::test_changed_krb_20 - assert 0.0 ± 1.0e-12 == -5.96
• FAILED tests/test_13_nsm_CBOD_calculations.py::test_changed_kbod_20 - assert 0.0 ± 1.0e-12 == -0.18
• FAILED tests/test_13_nsm_CBOD_calculations.py::test_changed_ksbod_20 - assert 0.0 ± 1.0e-12 == -0.4

Do you have ideas as to why this is or should we help dig into these tests?

[imscw95]

The first failed test - carbon_calculations - test_changed_kbod_20 - appeared to be a bug in RAS potentially. It was showing a matching value (.8048) after the second timestep, but showing the -45.77 value after the first timestep. The manual calcs show the .8048 value. I feel pretty solid that we are not way off with this one and would feel comfortable asserting = to .8048 with a note that there was a weird thing going on in the RAS modules.

The other errors are due to Kelsey's change that sets all negative values equal to 0 instead of letting them go negative. Seems like a reasonable change, though RAS allows negative concentrations. We could try to test other values for these tests so that we aren't running into this issue for now. I think these values were validated to RAS and previously passing so if there's a way to merge with these tests failing for now, I'm cool with that.

[PeterKlaver]

Negative concentrations are of course physically impossible, but they can be the result of computational issues such as too long of a time step. "Allowing" them at least permits the modeler to see that something is not right; setting them to zero just hides the problem temporarily. That may be enough in some cases but in others the simulation can either crash or (far worse IMO) proceed to completion with erroneous but plausible-looking results.

[sjordan29]

Thanks for your input @PeterKlaver -- we discussed on a check-in today and we think it makes sense to allow the negative numbers so the modeler can see the issue. @kewalak, are you able to update those processes accordingly (and hopefully then get the tests to pass)?

[aufdenkampe]

@kewalak, are you able to update the processes and tests as described by by @sjordan29 in #88 (comment)?

I would like to issue a release this week for the NSM work that we've accomplished to date and recently demoed to @TSteissberg .

[kewalak]

@aufdenkampe I will get it done by end of day tomorrow!

[kewalak]

@sjordan29 @aufdenkampe adjusted to allow for negative values. All tests are passing besides one in carbon and one in CBOD marked to fail with reason "known issue with kbod 20 test TBA".

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaf0b58) to head (b3ac970).
Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   89.13%   89.17%   +0.03%     
==========================================
  Files          25       25              
  Lines        1712     1709       -3     
==========================================
- Hits         1526     1524       -2     
+ Misses        186      185       -1     

Flag	Coverage Δ
	89.17% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sjordan29]

Thanks @kewalak. It looks like codecov is failing because the kelvin_to_celsius process is not covered by tests. This process hasn't been flagged in past reports, which I don't totally understand, but maybe it's only doing a check of lines that were changed, so the minor change to this process resulted in a flag? I see a few paths forward:

• Delete this process: my sense is that this process is not being tested, that means it is not being used in any of the other tests and we could probably delete it. Does that sound accurate? As I recall, TSM calculates change in temp in Kelvin [per this thread]. It doesn't look like the process is used anywhere in NSM either.
• Write a simple test for this process and put it somewhere in the testing suite, if we want to keep the conversion.
• Ignore the failure. I'd prefer not to do that, as then the main branch will show that certain tests are failing.

Any thoughts @aufdenkampe @kewalak?

[kewalak]

@sjordan29 @aufdenkampe I went ahead and removed it. It is not used anywhere and would be easy enough to just add back in at a later time.

[aufdenkampe]

@kewalak, thanks!! As soon as all tests pass, go ahead and merge.

I'll then issue a release. I have one drafted if any of you want to add any additional release notes.