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.

Sign up for GitHub

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

Jump to bottom

Create submodule files in LatLon #2899

Closed
wants to merge 10 commits into from
Closed

Create submodule files in LatLon #2899

wants to merge 10 commits into from

Conversation

JulesKouatchou
Copy link
Contributor

Types of change(s)

  • 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

  • Tested this change with a run of GEOSgcm
  • Ran the Unit Tests (make tests)

Description

  • Rename the folder latlon (in geom_mgr) as LatLon`.
  • Create submodule files in LatLon.
  • Change the version of ESMA_cmake from 3.46.0 to 3.37.0.

Related Issue

@JulesKouatchou JulesKouatchou added 0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) 📈 MAPL3 MAPL 3 Related Changelog Skip Skips the Changelog Enforcer labels Jul 11, 2024
@JulesKouatchou JulesKouatchou requested review from a team as code owners July 11, 2024 18:13
@JulesKouatchou
Copy link
Contributor Author

Made a mistake in the ESMA_cmake version number. It should have been 3.47.0.

tclune
tclune reviewed Jul 11, 2024
View reviewed changes
geom_mgr/LatLon/CMakeLists.txt Outdated
Comment on lines 14 to 15
SOURCES new_LatLonDecomposition_basic.F90 new_LatLonDecomposition_petcount.F90
new_LatLonDecomposition_topo.F90 get_lon_distribution.F90
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned in Teams - the constructors should stay in the module and not be split as submodules.

tclune
tclune reviewed Jul 11, 2024
View reviewed changes
geom_mgr/LatLon/CMakeLists.txt Outdated
esma_add_fortran_submodules(
TARGET MAPL.geom_mgr
SUBDIRECTORY LatAxis
SOURCES new_LatAxis.F90 equal_to.F90 not_equal_to.F90 supports_hconfig.F90
Copy link
Collaborator

Choose a reason for hiding this comment

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

put constructors back in module

tclune
tclune reviewed Jul 11, 2024
View reviewed changes
geom_mgr/LatLon/CMakeLists.txt Outdated
SUBDIRECTORY LonAxis
SOURCES equal_to.F90 get_lon_range.F90 make_LonAxis_from_metadata.F90
supports_hconfig.F90 get_lon_corners.F90 make_LonAxis_from_hconfig.F90
new_LonAxis.F90 supports_metadata.F90)
Copy link
Collaborator

Choose a reason for hiding this comment

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

constructors go in module

tclune
tclune requested changes Jul 11, 2024
View reviewed changes
Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

Constructors should stay in their original location. Generally these are functions that start with the new_ prefix.

Otherwise is fine.

@JulesKouatchou
Copy link
Contributor Author

Ok. I will make the changes right now. I will run tests before I push the changes.

@JulesKouatchou
Copy link
Contributor Author

Moved the constructors back to module files.

@tclune tclune self-requested a review July 12, 2024 12:35
tclune
tclune previously approved these changes Jul 12, 2024
View reviewed changes
@mathomp4
Copy link
Member

Hmm. Seems like a file is missing:

CMake Error at ESMA_cmake/esma_support/esma_add_*******_submodules.cmake:23 (target_sources):
  Cannot find source file:

    /root/project/workspace/build-MAPL/geom_mgr/LatLon/LatLonDecomposition_get_lon_distribution.F90

@tclune
Copy link
Collaborator

tclune commented Jul 12, 2024

It is on the main branch of ESMA_cmake (just checked). Maybe we need a CI update in this PR?

@mathomp4
Copy link
Member

It is on the main branch of ESMA_cmake (just checked). Maybe we need a CI update in this PR?

It should work. I pressed the update branch button just now to see.

@JulesKouatchou
Copy link
Contributor Author

I will close this PR. I will make a few edits to the branch and submit a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tclune tclune tclune left review comments

Assignees
No one assigned
Labels
0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) Changelog Skip Skips the Changelog Enforcer 📈 MAPL3 MAPL 3 Related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JulesKouatchou @mathomp4 @tclune