search

NOAA-CEFI-Regional-Ocean-Modeling / ocean_BGC

3 stars 4 forks source link

Cleanup idea: eliminate repeated diagnostic ID checks #14

Closed andrew-c-ross closed 2 months ago

andrew-c-ross commented 3 months ago

For generic tracer diagnostic writing, every time before data is posted, the code checks if the diagnostic ID is greater than 0 (if the diagnostic is being used). For example,

https://github.com/NOAA-CEFI-Regional-Ocean-Modeling/ocean_BGC/blob/bfa02118f1bda46cc230d18074401e6d782a8e74/generic_tracers/generic_COBALT.F90#L7586-L7589

I'm pretty sure this check could be moved to within the g_senddata functions within generic_tracerutils.F90. Doing so would allow the elimination of over 600 lines of `if (cobalt%id .gt. 0)` code from COBALT.

yichengt900 commented 3 months ago

@andrew-c-ross, thanks for the great cleanup ideas (including #15)! I believe both ideas are pretty straightforward and easy to implement. I could do both of them on my end, but I would love to use this opportunity to exercise our code review process and use this as an excellent example for other group members.

If you are okay with that, I am going to add a cleanup label and assign this issue to you, and you could open a PR associated with your cleanup idea. To save your time, you just need to fix one tracer diagnostic writing, and I can implement the others. By doing this, we will have a clean example for other developers.

@charliestock, do you think this is a good idea? Any thoughts are more than welcome!

andrew-c-ross commented 3 months ago

Sounds great, @yichengt900. Would also be good to get the opinion and advice of @nikizadehgfdl before we go about deleting all of these lines.

yichengt900 commented 3 months ago

Good point, @andrew-c-ross. @nikizadehgfdl, @andrew-c-ross has proposed two great ideas (this issue and #15) to eliminate repeated code in COBALT. We would love to hear your opinions before moving forward. Any inputs are more than welcome!

yichengt900 commented 3 months ago

The modified g_sed_data function has been merged through PR #16. Now we can consider implementing this new function to eliminate repeated ID check lines..

Also inspired by @nikizadehgfdl 's idea, we can further consider putting all send_diag calls in a separate subroutine to make the update_from_source subroutine easier to read. This change will also make our lives easier if we eventually decide to go with the route of semi-auto generation of diagnostic code (e.g., from a YAML file).

@charliestock, @andrew-c-ross, what are your thoughts? If you think this is a good way to go, I can work with @andrew-c-ross on this improvement.

yichengt900 commented 2 months ago

Addressed by PR #23. Close now