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

Fixes mpp_domains_copy #707

Merged
merged 6 commits into from
Mar 26, 2021
Merged

Fixes mpp_domains_copy #707

merged 6 commits into from
Mar 26, 2021

Conversation

uramirez8707
Copy link
Contributor

Description
This PR:

  1. Finishes implementing mpp_copy_domain, so that it can work correctly (Fixes mpp_copy_domain does not fully copy domain  #689)
  2. Creates a subroutine call mpp_create_super_grid that sets all of the indices correctly (Fixes supergrid creation helper function #690)
  3. Adds a unit test that tests if the copy and the supergrid worked correctly
    Without the source code changes the test fails on the mpp_get_compute_domains call on domain2 because domain2%list is never set ...

How Has This Been Tested?
make check passes
mpp_create_supergrid was implemented in NOAA-GFDL/FMScoupler#52 and it works now ...

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

Copy link
Contributor

@rem1776 rem1776 left a comment

Choose a reason for hiding this comment

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

Should the other overlapSpec pointers in domain2D get copied as well?

@@ -1693,16 +1742,29 @@ end subroutine mpp_get_tile_compute_domains
domain_out%pe = domain_in%pe
domain_out%pos = domain_in%pos

if (associated(domain_in%list)) then
Copy link
Member

Choose a reason for hiding this comment

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

Is this an actual pointer, or is it something that gets allocated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a pointer that gets allocated.

Copy link
Member

Choose a reason for hiding this comment

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

This should be changed to an allocatable then (in a different PR).

if (associated(domain_in%list)) then
starting = lbound(domain_in%list, 1)
ending = ubound(domain_in%list, 1)
allocate(domain_out%list(starting:ending))
Copy link
Member

Choose a reason for hiding this comment

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

Is this a pointer or an allocatable?

end subroutine mpp_set_super_grid_indices

!> @brief Modifies the indices of the input domain to create the supergrid domain
subroutine mpp_create_super_grid(domain)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this name could cause confusion as people might think you are creating a supergrid from domain information. I would suggest a name such as mpp_create_super_grid_domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renames to mpp_create_super_grid_domain in fde2cad

@rem1776 rem1776 added this to the 2021.02 milestone Mar 22, 2021
@rem1776
Copy link
Contributor

rem1776 commented Mar 24, 2021

@uramirez is this ready for review?

@uramirez8707
Copy link
Contributor Author

Should the other overlapSpec pointers in domain2D get copied as well?

@rem1776 I think those pointers are not required for any io stuff.

rem1776
rem1776 previously approved these changes Mar 24, 2021
@@ -161,6 +161,7 @@ module mpp_domains_mod
public :: mpp_clear_group_update
public :: mpp_group_update_initialized, mpp_group_update_is_set
public :: mpp_get_global_domains
public :: mpp_create_super_grid_domain
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a destructive operation, there should be doxygen information on how to utilize this function (copy, modify).

Comment on lines +290 to +293
!> @example This is an example of how to use mpp_create_super_grid_domain
!! call mpp_copy_domain(domain_in, domain_out)
!! call super_grid_domain(domain_out)
!! domain_in is the original domain, domain_out is the domain with the supergrid indices
Copy link
Member

Choose a reason for hiding this comment

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

@bensonr This addresses your comment Since this is a destructive operation, there should be doxygen information on how to utilize this function (copy, modify).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I should have been more specific about this information also being in mpp_domains.F90. I use that as my first place for all documentation on usage, etc. I'll approve and we can augment at a later date.

Comment on lines +290 to +293
!> @example This is an example of how to use mpp_create_super_grid_domain
!! call mpp_copy_domain(domain_in, domain_out)
!! call super_grid_domain(domain_out)
!! domain_in is the original domain, domain_out is the domain with the supergrid indices
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I should have been more specific about this information also being in mpp_domains.F90. I use that as my first place for all documentation on usage, etc. I'll approve and we can augment at a later date.

@rem1776 rem1776 merged commit 1d7d2f4 into NOAA-GFDL:main Mar 26, 2021
@uramirez8707 uramirez8707 deleted the supergrid branch January 25, 2022 16:10
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.

supergrid creation helper function mpp_copy_domain does not fully copy domain
5 participants