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
FEMSystem TimeSolver Fixes #1069
Conversation
Da fuq? These changes should not have had any effect on MOOSE. The debug failure a problem on the MOOSE testing side? |
"Max time exceeded", so probably just an extra-long-test that happened to be scheduled on an already-loaded node? |
I should've mentioned too that GRINS |
Roy is right however, this really shouldn't happen. We try to keep those On Wed, Aug 24, 2016 at 11:03 AM Paul T. Bauman notifications@github.com
|
@permcody I don't know if you invalidated this already or not... if so it failed again, if not, well, now it's invalidated. |
I'll rebase and push here. |
Otherwise, it's unnecessary since elem_fe_reinit is called before assembling each element residual contributions.
We need the call to reinit_func to set the time, t, in the context to the correct value. Also added clarifying comments in EulerSolver and Euler2Solver that we're also setting the time in addition to possibly resetting the mesh if there's mesh motion. There is probably a way to make this more efficient such that we only call reinit_func twice in Euler2Solver, but I didn't put any thought into it.
This test exercises the reinit_func fix. Before, we didn't have the correct time in the context so we didn't integrate this ODE exactly as we should have. Now we do.
We need to call reinit_func in order to set t = t_{n+1} for evaluating the residual for Newmark. Of course, we'll also need it for the mesh motion if it's there.
This is to prepare for adding a new test where we reset beta.
For this one, we should be able to integrate exactly a linear function (in time) of acceleration. This required both the beta setter (since you need beta = 1/6 for this case) and the reinit_func fix in the NewmarkSolver.
ec5be7f
to
2dd3138
Compare
Done. But it wouldn't have put the same commits in the history twice would it? They were the same hashes (before rebasing), I'd just created this branch from the tip of the previous one and I thought GitHub was just stupid. Am I the stupid one? |
You're right, it would have since those hashes didn't change. |
elem_position_set(theta); | ||
} | ||
elem_fe_reinit(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, but the doxygen comment for elem_reinit should probably be made clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean adding the note about setting the time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I mean making it clearer that we're not reinitializing FE objects unless a moving mesh requires it.
Originally elem_reinit was supposed to reinitialize FE objects; and if you look at the documentation in DiffSystem that's clearly implied. It wasn't until later optimization that I broke out elem_fe_reinit() as a separate method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I mean making it clearer that we're not reinitializing FE objects unless a moving mesh requires it.
Ah,OK.
Originally elem_reinit was supposed to reinitialize FE objects; and if you look at the documentation in DiffSystem that's clearly implied. It wasn't until later optimization that I broke out elem_fe_reinit() as a separate method.
Thanks for the clarification. I don't remember a time elem_fe_reinit wasn't called in the AssemblyContributions functor. But my memory has never been the same since my kids were born.
Everything's fine by me except for the now-atavistic elem_reinit() documentation. @pbauman can take a crack at rewriting that or I can try myself late tonight. fe32f8b is technically a big API change, but I suspect the changing API only ever gets used in the TimeSolver subclasses that @pbauman is currently in the process of fixing. |
I'll drop a PR in shortly.
Huh, that's a good point. I'd assumed it was a bug since elem_fe_reinit was called elsewhere and the <side,edge>_reinit versions had the <side,edge>_fe_reinit in the if-block.
I'd be shocked if this wasn't true. |
Builds on #1068.
There are several fixes here:
elem_fe_reinit
if there's mesh motion (3f3a757) (elem_fe_reinit
wasn't inside the if-block).reinit_func
calls inEuler2Solver
(eed32a9) andNewmarkSolver
(f28459). Also added comments clarifying that we're also setting the time in the context.NewmarkSolver
(fde77ae)Additional unit tests were added to cover testing the cases that were previously failing before the fixes.