NorESMhub / BLOM

Bergen Layered Ocean Model
GNU Lesser General Public License v3.0
16 stars 25 forks source link

prototype indent for hamocc using emacs #302

Closed mvertens closed 11 months ago

mvertens commented 11 months ago

prototype indent using emacs command 'f90-indent-subprogram'

jmaerz commented 11 months ago

Dear @mvertens , let us know, once you consider the PR as fully ready (since you added further commits, I am not sure about the status).

mvertens commented 11 months ago

I did my first pass to have indented files and all files be modules yesterday. I did a regression test with ERS_Ld3.T62_tn14.NOINYOC.betzy_intel.blom-hamocc1 and compared to the baselines and everything passed except the following 3 variables:

RMS dissic13lvl 3.9152E-12 NORMALIZED 1.6434E-10 RMS dissic14lvl 0.0000E+00 NORMALIZED 0.0000E+00 RMS delta13clvl 3.6645E-09 NORMALIZED 5.0047E-09

I then compared master to the baselines - and the same tests failed. So somewhere along the line these three variables changed answers. I'm not sure if this is expected or not. But it would be good to document this. Right now I'm in the process of stage 2 of the refactor which is making all modules private, documenting the arguments and downcasing some regions.

mvertens commented 11 months ago

Also - having the subroutine be upper case is making the routines look a bit odd - since use statements don't have upper case in them for the subroutines - so this leads to a bit of inconsistency. Same with the modules. Can I suggest that we back out this requirement?

mvertens commented 11 months ago

Also - I did push back my branch to the blom noresmhub repo - feature/emacs_indent_hamocc. If any of you have time, please have look at the first pass and let me know if you have any concerns.

jmaerz commented 11 months ago

Dear @mvertens , it depends, how old your baseline is. With #286, #287 and #290, I expect answer changes as compared to the master before for the mentioned isotope tracers.

mvertens commented 11 months ago

@jmaerz - my baselines were from Oct. 20.

jmaerz commented 11 months ago

Hi @mvertens , sorry for being a bit nit-picky, but does your master compiled as baserun on 20th already holds the PRs mentioned above or not (PR #290 was introduced at the 20th of October ...)

jmaerz commented 11 months ago

Wrt capitalization of subroutine and modules: that can be postponed and achieved later (changed easily).

mvertens commented 11 months ago

@jmaerz - you are not being picky. My master contains the latest master. My changes are bfb with the master. But the master is not bit-for-bit with the baseline from oct. 20. It looks like those answer changes are expected.

jmaerz commented 11 months ago

Dear @mvertens , if answers have changed before and after PR #290 (and #287, #286) for the isotope tracers, then this is indeed expected behavior and the HAMOCC crew (particularly Jörg) is aware of those changes. There shouldn't be any changes past #290 - which would mean a bug.

mvertens commented 11 months ago

@jmaerz - thanks. I don't think we have a problem.

jmaerz commented 11 months ago

Dear @mvertens , from my side no concerns thus far. Only observations: code indentation for follow up lines and for follow up comments have changed, which need at some point be reverted (e.g. best visible in mo_read_oafx). Further @JorgSchwinger , after these things have been done (and to just note it down right now): we should introduce a brief comment which logical controls go into which namelist in mo_control_bgc.F90.

mvertens commented 11 months ago

@jmaerz @JorgSchwinger @TomasTorsvik - I have just pushed back the second phase of refactorization. This includes:

I have verified that the changes are bit-for-bit for the test ERS_Ld3.T62_tn14.NOINYOC.betzy_intel.blom-hamocc1 compared to the latest master

Do you want me to add any other features like - DO -> do ENDDO -> enddo IF -> if ENDIF -> endif '=' -> ' = '

I'm happy to do that - but it will have to be tomorrow.

TomasTorsvik commented 11 months ago

Hi @mvertens I see that in some instances you have re-named files when you created modules (git-mv) (e.g. restart_hamoccwt.F90 -> mo_restart_hamocc.F90) and in other instances the files have been removed and re-introduced (e.g. powach.F90 -> mo_powach.F90). I think we want to use git-mv as much as possible in these cases to preserve the file history, at least if it's just adding a module wrapper around a subroutine. When I have done this before, I usually do the re-naming as a separate commit or with minimal changes. It may be difficult to do this retroactively on a branch (without rolling back or re-basing), but do you think it would be possible?

jmaerz commented 11 months ago

Hi @mvertens , I just realized that in ./trc/meson.build, the file extensions need to be adjusted to F90 for the renamed files to make the code meson-compilable.

mvertens commented 11 months ago

I am closing this PR in favor of a new one that has a cleaner implementation of the changes.