NOAA-CEFI-Regional-Ocean-Modeling / ocean_BGC

3 stars 4 forks source link

Automate maintenance of generic_COBALT diagnostics #22

Open kakearney opened 3 months ago

kakearney commented 3 months ago

This addresses some of the discussion in NOAA-CEFI-Regional-Ocean-Modeling/ocean_BGC#15 by automating the generation of source code related to the generic_COBALT_register_diag subroutine.

This update adds a few new files:

The idea here is that all maintenance of diagnostic registration is done by editing the .yaml file and rerunning the .py script, with no human ever directly editing generic_COBALT_register_diag_body.f90. (Down the road, we could get fancy and automate the re-running of the .py script any time an update was pushed to the .yaml file)

A few points for discussion:

Warning: I don't yet have my gaea account, so I have not tested the Fortran code for compilation! Someone please check my work, haha.

andrew-c-ross commented 3 months ago

This is amazing!

andrew-c-ross commented 3 months ago

I got this to compile and run a singe-column test case on my GFDL workstation. The only thing I had to do was change the file extension of the generated fortran file from .f90 to something else (.inc) so that the make script doesn't try to compile it on its own. (Then just the file extension of the file in the include line in generic_COBALT.f90 has to be changed to match).

andrew-c-ross commented 3 months ago

RE: include statements, one idea for an alternative would be to generate a complete module named something like generic_COBALT_utils that has a subroutine with all of the diagnostic registration code. Longer-term I think we can also automatically generate the diagnostic sending calls with just one addition to the yaml file, so it might be helpful to be able to put everything auto generated into one module file.

EDIT: I guess this could create some ugly circular dependencies though, and not sure how cobalt's use of module variables will impact that.

theresa-morrison commented 3 months ago

I think there are ways that we could add auto generated modules and manually generated modules to cobalt without introducing circular dependencies. It would definitely take a bit of work and involve moving things around within the ocean_BGC directory, but I think there are real advantages to not having all of cobalt in one 16,000 line .f90 file.

kakearney commented 3 months ago

Great to see all the feedback so soon! And I agree that a full discussion of how the code might be refactored would be good. There are many other places in the code (the g_tracer_add_param, g_tracer_add, g_tracer_get_values, g_send_data, and allocation and deallocation calls) that are similarly repetitive and might benefit from the same table -> autogenerated code treatment.

I updated my branch to fix the typos pointed out in the .py script and to change the extension of the autogenerated file to .inc.

yichengt900 commented 3 months ago

@kakearney, thanks! The updates look good to me. I will start the CI test. @charliestock, just keeping you in the loop. @kakearney opened a great PR as a starting point. I recommend we spend some time in next week's cobaltV3 meeting to further discuss how we are going to handle code refactoring.

yichengt900 commented 3 months ago

@kakearney, oops! It looks like the CI is complaining that the head commit for this pull request is not ahead of the base commit (dev/cefi repo). Could you please check and make sure your feature branch is up-to-date with the current main branch?

kakearney commented 3 months ago

Oops, overlooked the overnight (to me) update... should be synced now.

yichengt900 commented 3 months ago

Great job, @kakearney :1st_place_medal: ! The PR passed the CI test. I'll approve it for now, but let's hold off on merging until we discuss code refactoring in our next COBALTv3 meeting.