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

[LFRic] setval_random and setop_random do not work with redundant computation #2909

Open
arporter opened this issue Feb 26, 2025 · 4 comments
Assignees
Labels
bug LFRic Issue relates to the LFRic domain

Comments

@arporter
Copy link
Member

As discussed in #2805, setval_random cannot correctly perform redundant computation because there's no way of computing what pseudo-random values would be assigned to dof locations by neighbouring processors. This limitation applies both to annexed and halo dofs and therefore means that we cannot have COMPUTE_ANNEXED_DOFS=True in the configuration file.

I propose to put a check for this into PSyclone which will cause it to stop if it encounters either of the named kernels and COMPUTE_ANNEXED_DOFS is True. It will also refuse to apply the RedundantComputation transformation to loops containing these kernels.

@TeranIvy and @DrTVockerodtMO, I presume these kernels are only used in the adjoint test harness? (They were never intended for rigorous scientific use.)

@arporter arporter added bug LFRic Issue relates to the LFRic domain labels Feb 26, 2025
@DrTVockerodtMO
Copy link
Collaborator

By kernels do you mean whenever setval_random is invoked? In which case yes I believe this is only used by the adjoint_tests app.

I like the idea of refusing the redundant transformation calculation on setval_random but am less sure about the check, I'll explain why. Essentially the check will cause the exporting of adjoint itself to fail when used outside of the adjoint_tests app. This is how the current building behaviour works:

  1. The adjoint model is imported, and its import.mk script is ran.
  2. The import.mk script runs the PSyAd command, which generates both the adjoint kernels and the adjoint test algorithms (even if the latter is not used).
  3. The project working directory is PSycloned since otherwise the generated adjoint tests would not be PSycloned.

If we want to proceed in the way you suggest, then we would need to be careful as any project that imports the adjoint model will fail with the otherwise valid global psyclone.cfg script, merely because the adjoint tests were generated along with exporting the adjoint model (even if they won't be used).

I could create a dual behaviour in which the adjoint import optionally builds the adjoint tests (with the default set to false), and then the PSyAd command can check the config option only if the adjoint test is slated to be generated. I opted not to do this initially so that the adjoint build scripts were less complicated for when they eventually get ported to Fab.

@arporter
Copy link
Member Author

Yes, I mean whenever it is invoked. If it is invoked and COMPUTE_ANNEXED_DOFS is True then we will get wrong answers. The only safe way to handle this is to abort - if someone were to (ill advisedly) start using setval_random somewhere other than in the Adjoint testing, then that too would silently not work correctly. This wouldn't be picked up by a check in the psyad driver since psyad would not be used.
It seems to me that generating the adjoint tests will have to be a separate run of PSyAD with a separate config file. The other alternative would be to disable the distributed-memory option for the generation of the test harness but since you're currently trying to turn it on, I guess that's not an option you want :-)

@DrTVockerodtMO
Copy link
Collaborator

DrTVockerodtMO commented Feb 27, 2025

I think that allowing PSyAd to generate the test harness standalone would be best. That would certainly be a lot easier to modify the build scripts that I have as well!

@arporter Is this development planned for PSyclone 3.1.0 release?

@arporter
Copy link
Member Author

I'm afraid 3.1.0 is out of the door so this will be for the next release.

Having thought about it, we could have an (internal) option to disable the setval_random and setop_random unless we're doing test-harness generation.

As for generating the test harness only, you can always do -oad /dev/null to throw away the adjoint code if you wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug LFRic Issue relates to the LFRic domain
Projects
None yet
Development

No branches or pull requests

3 participants