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

Attempt to refactor use of FLAP. #1007

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

tclune
Copy link
Collaborator

@tclune tclune commented Aug 30, 2021

  • Migrated CapOptions into ./gridcomps/cap
  • Reduced use of #ifdef

Description

  • Relocated 2 files from ./base to ./gridcomps/Cap
  • Improved separation of CapOptions and FlapCLI layers.

Related Issue

Motivation and Context

Cleanup.

How Has This Been Tested?

Just MAPL tests. Further testing with GCM is warranted.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

tclune added 2 commits August 30, 2021 14:29
 - Migrated CapOptions into ./gridcomps/cap
 - Reduced use of `#ifdef`
@tclune tclune added 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 🛠️ Refactor This is code refactoring labels Aug 30, 2021
@tclune tclune requested a review from Gvilla1000-nasa August 30, 2021 19:09
@tclune tclune requested review from a team as code owners August 30, 2021 19:09
`make tests` builds most things, but not quite all.
@Gvilla1000-nasa Gvilla1000-nasa added the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Aug 31, 2021
@Gvilla1000-nasa
Copy link
Contributor

I don't see any changes here impacting the other refactoring PR, but I blocked it until the other one gets resolved.

Copy link
Member

@mathomp4 mathomp4 left a comment

Choose a reason for hiding this comment

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

Approved for CMake

@mathomp4
Copy link
Member

mathomp4 commented Sep 7, 2021

Oh wow. GNU does not like this. I wonder if it's a bug in the compiler?

@mathomp4
Copy link
Member

mathomp4 commented Sep 7, 2021

I'll attach a make.log here for your perusal:
make.log

use MAPL_ExceptionHandling
use mapl_KeywordEnforcerMod
use mapl_ExceptionHandling
use mapl_CapOptionsMod, only: MAPL_FlapCli => MAPL_CapOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the use of the name MAPL_FlapCli to rename the MAPL_CapOptions interface for the new_CapOptions procedure and still using it in this module as the name of the interface holding the new_CapOptions_from_flap procedure is causing conflicts. Commenting this line though makes it fail in the external components like Fvdycore.

@Gvilla1000-nasa
Copy link
Contributor

With the result from new_CapOptions_from_flap being now of type MAPL_CapOptions instead of MAPL_FlapCLI, wouldn't lines 21-22 of Tests/ExtDataDriver.F90 need to be modified from:

  cli = MAPL_FlapCLI(description='extdata driver',authors='gmao')
  cap_options=MAPL_CapOptions(cli)

to

 cap_options = MAPL_FlapCLI(description='extdata driver',authors='gmao')

and remove the type(MAPL_FlapCLI) :: cli?

@tclune
Copy link
Collaborator Author

tclune commented Sep 13, 2021

The intent was to avoid needing to edit the application programs which reside in other repos. (I.e., to preserve backwards compatibility. The second line is supposed to just to a copy now which makes it "silly" in the grand scheme of things, but lets those programs be unchanged. I'll try to poke around soon to see if this is a gfortran bug and/or if there is a workaround.

@mathomp4 mathomp4 merged commit c77efa1 into develop Nov 3, 2021
@mathomp4 mathomp4 deleted the feature/tclune/#996-cap-options-refactor branch November 8, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 🛠️ Refactor This is code refactoring 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants