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

Test nghost #855

Closed
apcraig opened this issue Aug 26, 2023 · 6 comments · Fixed by #913
Closed

Test nghost #855

apcraig opened this issue Aug 26, 2023 · 6 comments · Fixed by #913

Comments

@apcraig
Copy link
Contributor

apcraig commented Aug 26, 2023

Test nghost > 1, should be identical to nghost=1 for a given problem.

Alternatively, check value of nghost at initialization, abort if it's not 1.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 17, 2023

I ran some tests manually with nghost>1 and it runs fine and is bit-for-bit for standard gx3 configurations. We could

  • make nghost a namelist input
  • add nghost>1 tests to our test suite
  • extend some of our unit tests to comprehensively test things like halo updates with nghost>1

Right now, there is a large amount of code implemented using nghost, but we don't test any of it for ghost > 1.

On the other hand, I realize we only use nghost=1 (currently hardwired in code) and have no plans to have a larger stencil. So, we could also leave things as they are and defer any testing until it's needed, which might be never.

Thoughts?

@eclare108213
Copy link
Contributor

If it actually works (which surprises me!), then it would be nice to maintain it. I don't think we need to make nghost a namelist input until someone actually needs to change it. That said, maintaining it means testing that it doesn't get broken in some PR, which might require a namelist input. I'm also fine just leaving things as they are.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 18, 2023

I too was surprised the it even ran including with debug flags on. Then it produce bit-for-bit was even more shocking. It could be eap or vp or some other configurations don't work with nghost>1, so another step might be to extend the testing.

I'm undecided as well whether to test and maintain the nghost capability or just punt for now. If we do want to test, we need to make nghost a namelist so the testing software can run those tests.

If the likelihood that nghost is >1 anytime in the next decade or so is low, maybe we should just leave it alone.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 18, 2023

But from a software engineering perspective, it would be cool to add the necessary test cases and make it part of our test suite.

I guess the question is whether it's a requirement for the code to run with nghost>1. If so, we should test and maintain it. If not, then we shouldn't.

One way to view nghost is that it's a parameter set to 1 and it's not expected/allowed to change without further testing/development. The fact that a bunch of code supports nghost>1 doesn't mean it's a requirement to do so.

@eclare108213
Copy link
Contributor

I doubt that it will be needed, perhaps ever. It's only in there because CICE was built on POP's framework. We could just note in the documentation that it exists and if it needs to be used, then the developer needing it should take care to test it very thoroughly, and at that point add a namelist option for it.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 18, 2023

I agree, I'll try to add something to the documentation then will close this issue. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants