NOAA-CEFI-Regional-Ocean-Modeling / ocean_BGC

3 stars 4 forks source link

Clean up of the light and phytoplankton growth calculations #39

Closed charliestock closed 2 months ago

charliestock commented 2 months ago

This pull request is primarily a code clean up and improved commenting for the light and phytoplankton growth sections of the COBALT code. It was done in part to prepare for the discussion of these sections at our next COBALT "doc and dev" meeting.

I added some fairly extensive commenting that I am hoping could help spur the documentation part of this effort. I also eliminated substantial parts of the code that reflected different settings tested in the COBALTv3 photoacclimation analysis which were not ultimately used. While these eliminations were substantial, I regret to say that my comments were even more copious, resulting in a modest increase in the number of lines in the code. We'll need to decide on the proper penalty for this!

During the cleanup process I did discover that one parameter - ml_aclm_efold - was not quite defined as intended. It was intended to be the number of irradiance e-foldings that defined a deep mixed layer for photoacclimation. However, it was not coded in this way. I fixed this on line 8374 and changed the parameter value from 4.6 to 2.5 to maintain the equivalent behavior with the proper interpretation of the parameter.

Finally, since the overwhelming majority of the changes were deletions of commented out code or the addition of new comments, my testing was minimal. Perhaps we could develop a wiki entry with a few recommended configurations for testing before submitting a pull request? Apologies in advance for any hassles that arise, but if we could advance this pull request before the next "doc and dev", I think it will help the discussion considerably.

yichengt900 commented 2 months ago

Thanks, @charliestock . I can raise this PR for tomorrow's discussion. Currently, it failed the CI test. If you agree, I can work on your behalf and fix the bug in your feature branch so that I can approve and merge this PR.

charliestock commented 2 months ago

Hello Yi-Cheng,

I'm stuck in a meeting until 1 PM today, but lets discuss when we talk this afternoon. I suspect this issue is something small that I can fix quickly this afternoon or evening. Did the CI test complain about anything in particular?

yichengt900 commented 2 months ago

@charliestock, no worries. I also suspect the error should be small. I will check it, and perhaps I will be able to fix it from my side. However, I will let you review it before we move forward."

yichengt900 commented 2 months ago

Ok my patch fixes the model build process, but apparently the change of ml_aclm_efold and eqs changed our baseline answers and CI failed again. I will update our baselines.

I will add a runtime parameter change to our CI test to maintain the original answers for now. Once we confirm that this parameter change gives us satisfactory results from long-term hindcast run, I will update our baseline. @charliestock , are you okay with this approach?

yichengt900 commented 2 months ago

Oops, @charliestock plz ignore my previous comment. I missed your original comment. The changes are not only about the parameter itself, also it should be the number of irradiance e-foldings that defined a deep mixed layer for photoacclimation. In this case we will have to update baselines anyway. I will do that.