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

Make timekeeping module private by default #573

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

akturner
Copy link
Collaborator

Made timekeeping module private with explicit public statements for public members
Fixed bug in mpas_forcing which used mpas_timekeeping rather than mpas_derived_types in interfaces

Made timekeeping module private with explicit public statements for public members
Fixed bug in mpas_forcing which used mpas_timekeeping rather than mpas_derived_types in interfaces
@matthewhoffman
Copy link
Member

@akturner , this looks like helpful cleanup. I have a few questions about certain member of mpas_timekeeping being public (which I guess points to the need for this sort of cleanup).

  1. I'm not sure if daysInMonth is meant to be public. I see there is an identically named variable in src/core_ocean/mode_init/mpas_ocn_init_global_ocean.F.
  2. I'm not sure what abs is and if it should be public. It seems a bad idea for a routine name to match that of an intrinsic Fortran routine.
  3. I'm not sure if isLeapYear is meant to be public. I see it is used in src/core_atmosphere/physics/mpas_atmphys_manager.F and src/core_seaice/shared/mpas_seaice_column.F, but it does not follow MPAS convention of having public routines start with mpas_.

Apart from understanding those issues (and maybe @mgduda knows the answers), I'm good with this PR.

@mgduda
Copy link
Contributor

mgduda commented May 28, 2020

@akturner Having taken a quick scan through the changes, this all looks good. I'll do more thorough testing with a broader range of compilers soon.

I did find that we will need to fix a few module use statements in the atmosphere core in order to get it to compile after merging this PR. What I'm proposing is that I (or @MiCurry) make a PR to address these use statements in the atmosphere core, and to merge that PR before we merge this one. That way, there will not be a point on the develop branch where the atmosphere core fails to build.

@mgduda
Copy link
Contributor

mgduda commented May 28, 2020

@matthewhoffman The abs definition in mpas_timekeeping just overloads the abs intrinsic so that it also works with an MPAS_TimeInterval_type, in much the same way that we define the + and - operators for time intervals. So, I think we do want abs to be public.

Regarding the use of isLeapYear in mpas_atmphys_manager.F, it may be that we can rework whatever logic is currently using isLeapYear to make use of more abstract entities like time instants and time intervals that will automatically account for leap years when arithmetic on them is performed. I'll see what I can do in a reasonably short time frame.

@xylar
Copy link
Collaborator

xylar commented May 29, 2020

Regarding the use of isLeapYear in mpas_atmphys_manager.F, it may be that we can rework whatever logic is currently using isLeapYear to make use of more abstract entities like time instants and time intervals that will automatically account for leap years when arithmetic on them is performed.

It seems like isLeapYear should probably just remain public if the atmosphere and seaice cores are using it. Perhaps with a name change to add the mpas_ prefix?

@matthewhoffman
Copy link
Member

I'm certainly not opposed to isLeapYear remaining public, I just wanted to ensure that it was meant to be used in that way before officially supporting it. I like Xylar's idea of adding the mpas_ prefix and calling it good.

@mgduda
Copy link
Contributor

mgduda commented Jun 11, 2020

As of PR #582, we've addressed issues in the atmosphere core that would lead to build errors after the merge of this PR.

I like the idea of adding an mpas_ prefix to isLeapYear to maintain the convention that public entities begin with mpas_. I'd imagine a single additional commit that makes this change across the entire code base would be appropriate, as that would ensure there is no commit that doesn't compile.

@matthewhoffman
Copy link
Member

@akturner , it looks like this PR has fallen through the cracks. Is this something that we should still try to merge? Or should it be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants