Skip to content

Remove landIceMask from diagnostics computation#672

Merged
mattdturner merged 1 commit intoMPAS-Dev:ocean/developfrom
xylar:ocean/remove_diagnostic_land_ice_mask
Sep 1, 2020
Merged

Remove landIceMask from diagnostics computation#672
mattdturner merged 1 commit intoMPAS-Dev:ocean/developfrom
xylar:ocean/remove_diagnostic_land_ice_mask

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Aug 28, 2020 •
edited
Loading

This was removed in #447 but inadvertently got added back in in #457.

It should be the first step toward addressing E3SM-Project/E3SM#3797

@xylar
Remove landIceMask from diagnostics computation
f47b037 
This was removed earlier but inadvertently got added back in.
@xylar
Copy link
Collaborator Author

xylar commented Aug 28, 2020

Testing

I ran the QU240wISC test case through the forward run on my laptop. That proves to me that the code compiles and runs with ice-shelf cavities without issues but it does not prove that it addresses E3SM-Project/E3SM#3797.

@jonbob or @darincomeau, would it be easy for you to cherry-pick this fix and see if it helps?

@mattdturner
Copy link
Collaborator

mattdturner commented Aug 28, 2020

@xylar Thanks for taking care of this. I just re-reviewed all of the changes in #457, and as far as I can tell this appears to be the only place were prior changes were reverted.

Approved via visual inspection.

@jonbob
Copy link
Contributor

jonbob commented Aug 28, 2020

@xylar - I'll cherry-pick this right now and see if it fixes our problem

@mark-petersen
Copy link
Contributor

mark-petersen commented Aug 28, 2020

Thanks, @xylar that is a big help. I looked through the files before and after #457 was merged, and I agree with your assessment. This code was added back in during a complicated rebase.

We will shoot for merging this in Monday after some testing in E3SM.

@jonbob
Copy link
Contributor

jonbob commented Aug 28, 2020

@xylar - that did fix the issue with the ECwISC30to60E1r2 mesh. Thank you so much for jumping in

jonbob
jonbob approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@jonbob jonbob left a comment

Choose a reason for hiding this comment

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

Tested by cherry-pick and verified that it fixes our observed issue

@xylar
Copy link
Collaborator Author

xylar commented Aug 28, 2020

@mark-petersen, any chance of getting this in to E3SM today?

darincomeau
darincomeau approved these changes Aug 28, 2020
View reviewed changes
Copy link

@darincomeau darincomeau left a comment

Choose a reason for hiding this comment

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

I can confirm this worked for me in resolving the domain issue we were seeing with ECwISC30to60E1r2. Thanks!!

mark-petersen
mark-petersen approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

Approved based on visual inspection of this code, previously removed. Also ran nightly regression suite and QU240wISC sanity check, all match bfb with ocean/develop using gnu optimized on grizzly.

@mark-petersen
Copy link
Contributor

mark-petersen commented Aug 28, 2020

We will plan to merge this on Monday.

@mattdturner mattdturner mentioned this pull request Aug 28, 2020
Closed
@mattdturner mattdturner merged commit c5229d9 into MPAS-Dev:ocean/develop Sep 1, 2020
@darincomeau darincomeau mentioned this pull request Sep 1, 2020
Closed
@mattdturner mattdturner mentioned this pull request Sep 1, 2020
Merged
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Sep 1, 2020
@jonbob
Merge branch 'mattdturner/ocean/fix_landIceMaxk_bug' into next (PR #3806
310789e 
)

Update mpas-source: fix landIceMask bug

This PR brings in a new mas-source submodule with changes only to the ocean
core. It fixes a bug in the MPAS-O landIceMask computation in diagnostics.

See MPAS PR MPAS-Dev/MPAS-Model#672

Fixes #3797

[BFB]
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Sep 2, 2020
@jonbob
Merge branch 'mattdturner/ocean/fix_landIceMaxk_bug' (PR #3806)
f1ce235 
Update mpas-source: fix landIceMask bug

This PR brings in a new mas-source submodule with changes only to the ocean
core. It fixes a bug in the MPAS-O landIceMask computation in diagnostics.

See MPAS PR MPAS-Dev/MPAS-Model#672

Fixes #3797

[BFB]
@xylar xylar deleted the ocean/remove_diagnostic_land_ice_mask branch September 10, 2020 07:23
mark-petersen pushed a commit to mark-petersen/MPAS-Model that referenced this pull request Jan 11, 2021
@mattdturner
Merge PR MPAS-Dev#672 'xylar/ocean/remove_diagnostic_land_ice_mask' i…
7da6d19 
…nto ocean/develop

This was removed in MPAS-Dev#447 but inadvertently got added back in in MPAS-Dev#457.

It should be the first step toward addressing E3SM-Project/E3SM#3797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mark-petersen mark-petersen mark-petersen approved these changes

@mattdturner mattdturner mattdturner approved these changes

+2 more reviewers

@darincomeau darincomeau darincomeau approved these changes

@jonbob jonbob jonbob approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mattdturner mattdturner

Labels

bug Ocean

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@xylar @mattdturner @jonbob @mark-petersen @darincomeau @matthewhoffman