Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prototype indent for hamocc using emacs #302

Closed
wants to merge 5 commits into from

Conversation

mvertens
Copy link
Contributor

@mvertens mvertens commented Nov 6, 2023

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

@jmaerz
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

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
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

@jmaerz - my baselines were from Oct. 20.

@jmaerz
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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

@mvertens
Copy link
Contributor Author

mvertens commented Nov 7, 2023

@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
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

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

'mo_accfields.F90',
'mo_aufr_bgc.F90',
'mo_aufw_bgc.F90',
'mo_mo_ini_fields.F90',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mvertens , why mo_mo_ini_fields? - one mo_ is enough :-) - might be the cause for the failing tests on github.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the typo. I'm not using meson build - but the NorESM CIME build. I'll fix this in my upcoming push.
Thanks for catching this.

@jmaerz
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

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

  • blocks detailing history now have functionality at the top and history condensed below as well as
    no longer contains parameter documentation - those have been moved to the parameter declarations themselves
  • private, public declarations declared consistently across modules
  • implicit none only declared at the module level
  • no save statements at the module level
  • WRITE->write
  • ALLOCATE->allocate
  • CALL ->call
  • lines that don't need to be broken up are no longer broken up

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
Copy link
Contributor

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
Copy link
Collaborator

jmaerz commented Nov 7, 2023

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
Copy link
Contributor Author

mvertens commented Nov 7, 2023

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

@mvertens mvertens closed this Nov 7, 2023
@mvertens mvertens deleted the feature/emacs_indent_hamocc branch November 9, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants