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

v23 picard changes #801

Open
wants to merge 10 commits into
base: devel
Choose a base branch
from
Open

Conversation

aprilnovak
Copy link
Collaborator

Same PR as #790, but I did not want to overwrite @regerdavid's branch to split out into 2 PRs.

@moosebuild
Copy link
Collaborator

Job Test on a313c7d wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Collaborator

moosebuild commented Oct 12, 2023

Job Coverage on 6ebd385 wanted to post the following:

Coverage

3ef287 #801 6ebd38
Total Total +/- New
Rate 93.40% 93.07% -0.33% 43.14%
Hits 7654 7664 +10 22
Misses 541 571 +30 29

Diff coverage report

Full coverage report

Warnings

  • New new line coverage rate 43.14% is less than the suggested 90.0%

This comment will be updated on new commits.

nekrs::finishStep();
const PostprocessorValue * iter = &getPostprocessorValueByName("fp_iteration");
std::cout << "Current fixed point iteration number read in NekRSProblemBase is " << *iter
<< std::endl; // JUST FOR TESTING
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being kept in the final code? Comment indicates otherwise

Choose a reason for hiding this comment

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

I think this should be kept in the final code. Depending on how the postprocessor for the iteration is passed into nek.i, it can be possible to mess it up (if the correct execute_on flags are not used), so I think this is necessary for the user to check that the correct iteration count is being received.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the comment should be removed, otherwise everything looks good to me, thanks @regerdavid @aprilnovak

{
converged = nekrs::runStep(corrector++);
} while (!converged);
// TODO: time is somehow corrected here
Copy link
Contributor

@anshchaube anshchaube Oct 21, 2023

Choose a reason for hiding this comment

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

Has the TODO item been accomplished i.e. the time correction? @regerdavid

Choose a reason for hiding this comment

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

I don't believe that that is my TODO comment (looks like it exists in the current master branch of cardinal) so I'm not sure if that is resolved or not.

nekrs::finishStep();
const PostprocessorValue * iter = &getPostprocessorValueByName("fp_iteration");
std::cout << "Current fixed point iteration number read in NekRSProblemBase is " << *iter
<< std::endl; // JUST FOR TESTING
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<< std::endl; // JUST FOR TESTING
<< std::endl;

@regerdavid
Copy link

As an update, further testing is needed to make sure that the fixed point iterations are working correctly for non-FSI applications. I was trying to use them with a CHT case today and was not getting expected behavior.

@regerdavid
Copy link

Following up on my previous comment, the issue with CHT is related to a bug in NekRS that currently exists. This is supposedly fixed in next, but I have not gotten to try it yet
Nek5000/nekRS#516

@aprilnovak
Copy link
Collaborator Author

This is finally ready to consider for merging, now that the CHT bug has been fixed in NekRS's next (which I updated last week). However, I'd like to see at least one test for these changes to make sure we maintain this functionality. @regerdavid is this something you/another student are still working on? What would you like to see happen with this pull request?

@moosebuild
Copy link
Collaborator

Job Documentation on 6ebd385 wanted to post the following:

View the site here

This comment will be updated on new commits.

@regerdavid
Copy link

I'm not actively working on making a test for it, but I can try to come up with a simple test case for it. If there is one thing I'd like to see, its to get a change/addition in the execute_on flags in MOOSE so that we have one that will allow the Picard iterations to work properly without having to manually add an EXEC_CUSTOM flag.

@anshchaube
Copy link
Contributor

I think I have found a workaround for the execute_on flags, but some issues still remain (with the number-of-iteration postprocessor transfer lagging at the first fixed point iteration,and some issues with subcycling and data-transfers). The path forward for that would require attempting to reproduce these issues without NekRS. I can take a look at this in the coming weeks.

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.

4 participants