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

(*)Fix bug in rotate_ALE_sponge for fixed sponges #667

Merged
merged 1 commit into from
Jul 9, 2024
Merged

(*)Fix bug in rotate_ALE_sponge for fixed sponges #667

merged 1 commit into from
Jul 9, 2024

Conversation

Hallberg-NOAA
Copy link
Member

Corrected a bug in rotate_ALE_sponge() that incorrectly set the starting number of sponge fields that prevented any cases with fixed ALE sponges from working when ROTATE_INDEX = true, even if INDEX_TURNS = 0. With this correction, the code is now working (and giving identical answers) with rotation enabled for cases with fixed ALE_sponges. All answers are bitwise identical in cases that worked before, but some cases that did not work at all are now working both with and without grid rotation.

@Hallberg-NOAA Hallberg-NOAA added the bug Something isn't working label Jun 7, 2024
theresa-morrison
theresa-morrison approved these changes Jun 24, 2024
View reviewed changes
Copy link

@theresa-morrison theresa-morrison left a comment

Choose a reason for hiding this comment

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

This looks like a simple change that addresses the issue.

@marshallward
Copy link
Member

I just approved this since it seems necessary, but I am confused by sponge_in%fldno was not also set to zero. This seems like it should have been resolved upstream.

@marshallward
Copy link
Member

Since initialize_ALE_sponge_varying sets sponge%fldno to zero, it could make more sense for initialize_ALE_sponge_fixed to also set fldno to zero.

@Hallberg-NOAA
(*)Fix bug in rotate_ALE_sponge for fixed sponges
424720f 
  Corrected a bug in rotate_ALE_sponge that incorrectly set the starting number
of sponge fields that prevented any cases with fixed ALE sponges from working
when ROTATE_INDEX is true, even if INDEX_TURNS is 0.  With this correction, the
code is now working (and giving identical answers) with rotation enabled for
cases with fixed ALE_sponges.  All answers are bitwise identical in cases that
worked before, but some cases that did not work at all are now working both
with and without grid rotation.
@Hallberg-NOAA Hallberg-NOAA force-pushed the fix_fixed_ALE_sponge_rotation branch from 29d944d to 424720f Compare July 9, 2024 08:02
@marshallward
Copy link
Member

I was mistaken, a fixed sponge sponge_in has already been set up, so its sponge_in%fldno is nonzero. If I had done some kind of "sponge copy" like with the varying sponge, the current version would be correct. But (for reasons I no longer remember) I chose to repeat the fixed sponge setup which needs fldno to be reset to zero.

Perhaps there is a better design in here somewhere, but no need to overthink it for now. I agree that this is correct.

@marshallward
Copy link
Member

One last thought: You might be able to just remove sponge%fldno = 0 since the sponge type initializes this to zero. But setting it again can't hurt.

@Hallberg-NOAA
Copy link
Member Author

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/24042.

@Hallberg-NOAA Hallberg-NOAA merged commit 659d19b into NOAA-GFDL:dev/gfdl Jul 9, 2024
10 checks passed
This was referenced Nov 27, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theresa-morrison theresa-morrison theresa-morrison approved these changes

@marshallward marshallward marshallward approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hallberg-NOAA @marshallward @theresa-morrison