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

Update areafac_c, areafac_ce in halo in dynamics #1

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

apcraig
Copy link

@apcraig apcraig commented Sep 14, 2023

I believe this fixes the issues in the updated remap advection. The problem was that CICE was not bit-for-bit on different block sizes with the latest changes even though it was bit-for-bit for a fixed block size with different pes or decompositions. This indicated a problem on the halo. The problem existed with B, C, or CD grids in this branch.

The fix is to change the areafac_c and areafac_ce fields so they are defined everywhere, including in the complete halo. The current version only sets areafac_c and areafac_ce on part of the halo (for reasons I don't understand). earea and narea are defined everywhere on the halo, so we can set areafac_c and areafac_ce in the complete halo trivially.

The changes in ice_grid.F90 are not required for this fix, but are probably needed for symmetry testing (which we don't do automatically yet).

Somewhere in the remap advection special cases, areafac_c or (more likely) areafac_ce must have been used on a point where it wasn't set properly on the halo. And that's why different block sizes produced different answers. With these fixes, that problem goes away. That doesn't guarantee that all the special cases are implemented correctly in terms of offsets and answers, but this does fix the block size reproducibility issue.

I'm running a complete test suite now and will have more results soon. Feel free to merge and/or test this in the areafact branch in the mean time. I think it's safe to do so. I will run a complete test suite, we probably want to also rerun the QCs and any other manual testing just be really sure the final thing we want to merge is OK.

@JFLemieux73
Copy link
Owner

Thank you very much Tony. I am not sure I understand everything but I will merge this and redo the tests.

@JFLemieux73 JFLemieux73 merged commit 1a4fd67 into JFLemieux73:Areafact Sep 14, 2023
1 check passed
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.

2 participants