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

Remove unused parameters in wrt* subroutine calls in ncout_hamocc.F90 #222

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Remove unused parameters in wrt* subroutine calls in ncout_hamocc.F90 #222

merged 1 commit into from
Jan 10, 2023

Conversation

JorgSchwinger
Copy link
Contributor

Since PNETCDF has been introduced, attributes to netCDF output variables are defined in hamoccvardef and not longer during writing the fields to file in the wrt* subroutines defined in mo_bgcmean.F90. I have now removed these no longer used arguments, which avoids confusion and improves readability of ncout_hamocc quite a bit.

Copy link
Collaborator

@jmaerz jmaerz left a comment

Choose a reason for hiding this comment

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

Dear Jörg, looks good to me - I haven't checked each individual factor, though. So I approve. The only thing that is getting a bit puzzling to me is the further detachment of variables and units (as in: the factors set in wrtX is even less clear now... - my first reaction was wishing for a unit comment :-) ). When looking into your code changes, I started to ponder, whether it would be possible to write a wrapper function that combines i) declaration of netcdf variable ii) closing of iogrp, iii) the writing of the actual variable to netcdf and iv) re-opening of the iopgr. Maybe we can discuss this.

@JorgSchwinger
Copy link
Contributor Author

Hi Jöran, everything is still there - in the hamoccvardef subroutine, so you can still find the variables together with the long names and units. This PR removes only duplicate (and unused) stuff.

One could think of combining ncout_hamocc and hamoccvardef into mo_bgcmean (or creating a new module mo_ncout_hamocc)

@jmaerz
Copy link
Collaborator

jmaerz commented Jan 9, 2023

Hi Jörg, sure, I know. From my side just go ahead as approved. My point is that one sets a factor somewhere and defines the nc variables units somewhere else (while indeed that was the case before, as I now understand better after your changes - I also got confused about the doubling of the information that we provided before - so in that way, it's clearer now). Thanks for the work!

@JorgSchwinger
Copy link
Contributor Author

Mhm yes, I see the point with the units and the conversion factor. I agree that this not nice, although I am not sure if there is a way around it (I think with PNETCDF everything has to be defined before any processor starts writing into the file, while the conversion factor can only be applied at the time of writing).

I'll see if Tomas has something and go ahead with merging this.

Copy link
Contributor

@TomasTorsvik TomasTorsvik left a comment

Choose a reason for hiding this comment

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

Hi @JorgSchwinger , this loks fine to me. I don't have anything to add beyond what was already commented.

@JorgSchwinger JorgSchwinger merged commit 440de2d into NorESMhub:master Jan 10, 2023
@JorgSchwinger JorgSchwinger deleted the feature-clean_ncouthamocc branch January 10, 2023 08:20
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