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 get_fptr_shape public and a bug fix #3118

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

pchakraborty
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)
  • Refactor (no functional changes, no api changes)

Checklist

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

Description

  • Excluding negative values of gridToFieldMap
  • Making mapl3g_FieldCondensedArray::get_fptr_shape public

Related Issue

2. Making get_fptr_shape public
@pchakraborty pchakraborty added 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 📈 MAPL3 MAPL 3 Related Changelog Skip Skips the Changelog Enforcer labels Oct 23, 2024
@pchakraborty pchakraborty requested a review from a team as a code owner October 23, 2024 23:36
@pchakraborty pchakraborty self-assigned this Oct 24, 2024
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.

I would still like to see a unit test that guards against this issue. This is the 2nd bug that has been found in this interface. First was that the gridToFieldMap was not even being allocated, and the unit tests only covered the lower level procedures. (@darianboggs)

@pchakraborty
Copy link
Contributor Author

I would still like to see a unit test that guards against this issue. This is the 2nd bug that has been found in this interface. First was that the gridToFieldMap was not even being allocated, and the unit tests only covered the lower level procedures. (@darianboggs)

Created issue #3127

@pchakraborty pchakraborty merged commit d3bd437 into release/MAPL-v3 Oct 25, 2024
36 checks passed
@pchakraborty pchakraborty deleted the bug/pchakrab/field-utils branch October 25, 2024 15:41
Copy link
Contributor

@darianboggs darianboggs left a comment

Choose a reason for hiding this comment

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

It looks good.

I'm not sure what the use case for get_fptr_shape being public is, but I'm not opposed. The intention was that assign_fptr_condensed_array would get the fptr without the user needing to know the shape. get_fptr_shape is used to get the shape according to the Condensed Array convention. assign_fptr_condensed_array uses assign_fptr in FieldPointerUtilities.F90 to get the fptr. Once you have the fptr, you can get the shape with the Fortran shape function.

In other words, FieldCondensedArray.F90 depends on FieldPointerUtilities.F90, so you do not need to use both to get the fptr, and you don't need get_fptr_shape to get the shape of the fptr outside of FieldCondensedArray.F90

We could move all of FieldCondensedArray.F90 to FieldPointerUtilities.F90 with assign_fptr_condensed_array public and get_fptr_shape private.

FieldCondensedArray_private.F90 is internal to field_utils with public methods for the sake of unit testing.

Let me know if there is an unmet need.

@tclune
Copy link
Collaborator

tclune commented Oct 25, 2024

I'm not sure what the use case for get_fptr_shape being public is, but I'm not opposed.

I think you are out of date. That was removed.

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. Changelog Skip Skips the Changelog Enforcer 📈 MAPL3 MAPL 3 Related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants