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

GPU support for averaging #48

Closed
wants to merge 4 commits into from
Closed

GPU support for averaging #48

wants to merge 4 commits into from

Conversation

marc-85
Copy link
Contributor

@marc-85 marc-85 commented Aug 9, 2021

With these modifications averaging happens now on GPU with no data movement CPU-GPU.
The tests uses the same reference solutions as those used for the CPU tests. Tests performed with multiple MPI processes are skipped when not enough GPUs are found

mean pressure variable named as `mean-E' instead of `mean-p'
src/M2ulPhyS.cpp Outdated
@@ -306,7 +306,7 @@ void M2ulPhyS::initVariables()
if(dim == 3)
{
ioData.registerIOVar("/meanSolution","mean-w",3);
ioData.registerIOVar("/meanSolution","mean-E",4);
ioData.registerIOVar("/meanSolution","mean-p",4);
Copy link
Contributor

Choose a reason for hiding this comment

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

will this impact restart capability for runs we have ongoing that are saving a variable named /meanSolution/mean-E presently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will but only if you restart with the option CONTINUE_MEAN_CALC. If that is the case, I guess, one possibility would be to change the name of the variable on the .h5 file directly with a script. If not possible I can work out something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming our current runs would want to continue using CONTINUE_MEAN_CALC. So, holding off on landing this - I will need to create a companion script where we can update the name in existing files prior to restarting.

Is this just a naming error, ie, it has been pressure since the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's just the name. The variable stored is pressure

@marc-85 marc-85 mentioned this pull request Aug 13, 2021
@trevilo
Copy link
Contributor

trevilo commented Jul 7, 2022

Closing because this is too stale. It is unclear to me exactly what change was intended here. The title and first comment imply that it contains an extention of the averaging on the gpu, but from the commits referenced I don't see it. But... the diff is also very weird, since somehow github thinks 0 files have been changed.

Regardless, we are doing averaging on the gpu (as of #67), so at least that part has been handled.

The only non-merge, non-style commit here appears to be 4bbb6c7, which fixes a variable name in the mean. Since this PR is too stale to be useful otherwise, I'm closing it and opening an issue about the name (#157).

@trevilo trevilo closed this Jul 7, 2022
@trevilo trevilo deleted the average-gpu branch August 26, 2023 02:27
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.

3 participants