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

Adding a postprocessor to plot relative solution norm #7133

Merged
merged 1 commit into from Jun 3, 2016

Conversation

Projects
None yet
3 participants
@andrsd
Contributor

andrsd commented Jun 1, 2016

Local variable in Transient is made into a member variable. A new postprocessor is added to access this variable.
The computation of the norm was moved from keepGoing to solveStep so that we have the norm available at the end of the timestep. Otherwise the values in the postprocessor were delayed by 1 time step.

Closes #7118

@YaqiWang

This comment has been minimized.

Show comment
Hide comment
@YaqiWang

YaqiWang Jun 1, 2016

Contributor

@andrsd Just make you be aware of this Executioner::addAttributeReporter and the postprocessor ExecutionerAttributeReporter.C.

Contributor

YaqiWang commented Jun 1, 2016

@andrsd Just make you be aware of this Executioner::addAttributeReporter and the postprocessor ExecutionerAttributeReporter.C.

@andrsd

This comment has been minimized.

Show comment
Hide comment
@andrsd

andrsd Jun 1, 2016

Contributor

Marking Under development until I fix the recovery tests...

Contributor

andrsd commented Jun 1, 2016

Marking Under development until I fix the recovery tests...

@andrsd

This comment has been minimized.

Show comment
Hide comment
@andrsd

andrsd Jun 1, 2016

Contributor

Fixed the recovery issue.

@YaqiWang Thanks for telling me about Executioner::addAttributeReporter, I had no idea it existed. However, I think I prefer a postprocessor rather then a flag that we would turn on/off. Similary like we have TimeStepSize, NumDofs, NumElems, NumLinearIterations, etc.

Contributor

andrsd commented Jun 1, 2016

Fixed the recovery issue.

@YaqiWang Thanks for telling me about Executioner::addAttributeReporter, I had no idea it existed. However, I think I prefer a postprocessor rather then a flag that we would turn on/off. Similary like we have TimeStepSize, NumDofs, NumElems, NumLinearIterations, etc.

Show outdated Hide outdated framework/src/executioners/Transient.C
// Compute new time solution l2_norm
Real new_time_solution_norm = _problem.getNonlinearSystem().currentSolution()->l2_norm();
// Compute l2_norm relative error
_ss_relerr_norm = fabs(new_time_solution_norm - _old_time_solution_norm)/new_time_solution_norm;

This comment has been minimized.

@friedmud

friedmud Jun 1, 2016

Contributor

Actually... this isn't the right thing to do! I can see that it's the same formula as before... but it's just not right (and this code might be from me).

The formula SHOULD be:

norm(new_u - old_u) / norm(new_u)

What we currently have is:

(norm(new_u) - norm(old_u)) / norm(new_u)

Which is just plain wrong!

@friedmud

friedmud Jun 1, 2016

Contributor

Actually... this isn't the right thing to do! I can see that it's the same formula as before... but it's just not right (and this code might be from me).

The formula SHOULD be:

norm(new_u - old_u) / norm(new_u)

What we currently have is:

(norm(new_u) - norm(old_u)) / norm(new_u)

Which is just plain wrong!

This comment has been minimized.

@andrsd

andrsd Jun 2, 2016

Contributor

Yes, we would need to use a PPS for this.
I believe it is this way, because at the time you put that in there were no postprocessors...

@andrsd

andrsd Jun 2, 2016

Contributor

Yes, we would need to use a PPS for this.
I believe it is this way, because at the time you put that in there were no postprocessors...

This comment has been minimized.

@andrsd

andrsd Jun 2, 2016

Contributor

Um, no PPS necessary, I was thinking something else...

@andrsd

andrsd Jun 2, 2016

Contributor

Um, no PPS necessary, I was thinking something else...

@andrsd

This comment has been minimized.

Show comment
Hide comment
@andrsd

andrsd Jun 2, 2016

Contributor

I pushed a patch that fixes the formula, only the new test diffed, so I re-golded it. Modules were clean, but let's see how about the apps...

Contributor

andrsd commented Jun 2, 2016

I pushed a patch that fixes the formula, only the new test diffed, so I re-golded it. Modules were clean, but let's see how about the apps...

Show outdated Hide outdated framework/src/executioners/Transient.C
NonlinearSystem & nl = _problem.getNonlinearSystem();
NumericVector<Number> & current_solution = (*nl.sys().current_local_solution);
NumericVector<Number> & old_solution = (*nl.sys().old_local_solution);
NumericVector<Number> & difference = *NumericVector<Number>::build(_communicator).release();

This comment has been minimized.

@friedmud

friedmud Jun 2, 2016

Contributor

This isn't really a good idea... could be really slow in parallel to re-allocate this every time step

Better idea: add a "work" vector to the nonlinear system to use for this purpose. Add it as PARALLEL... and it doesn't need to be projected during adaptivity

@friedmud

friedmud Jun 2, 2016

Contributor

This isn't really a good idea... could be really slow in parallel to re-allocate this every time step

Better idea: add a "work" vector to the nonlinear system to use for this purpose. Add it as PARALLEL... and it doesn't need to be projected during adaptivity

This comment has been minimized.

@andrsd

andrsd Jun 2, 2016

Contributor

Yeah I know, I was just testing you. I borrowed it from your code in FEProblem::solutionChangeNorm() ;-D

@andrsd

andrsd Jun 2, 2016

Contributor

Yeah I know, I was just testing you. I borrowed it from your code in FEProblem::solutionChangeNorm() ;-D

This comment has been minimized.

@friedmud

friedmud Jun 2, 2016

Contributor

Hmm - yeah - are we already computing this there? Can you just get the value from there or call that code (and we can change that code to be better too ;-)

@friedmud

friedmud Jun 2, 2016

Contributor

Hmm - yeah - are we already computing this there? Can you just get the value from there or call that code (and we can change that code to be better too ;-)

This comment has been minimized.

@andrsd

andrsd Jun 2, 2016

Contributor

It is a different formula there, we divide by dt there...

@andrsd

andrsd Jun 2, 2016

Contributor

It is a different formula there, we divide by dt there...

This comment has been minimized.

@andrsd

andrsd Jun 2, 2016

Contributor

Otherwise, I'd just reuse the value from there...

@andrsd

andrsd Jun 2, 2016

Contributor

Otherwise, I'd just reuse the value from there...

This comment has been minimized.

@friedmud

friedmud Jun 2, 2016

Contributor

You'll have to deprecate the existing one... quite a few people use it.

If I remember correctly there is other "naming nonsense" involved with those parameters... let's try to get them right this time since we're changing them...

@friedmud

friedmud Jun 2, 2016

Contributor

You'll have to deprecate the existing one... quite a few people use it.

If I remember correctly there is other "naming nonsense" involved with those parameters... let's try to get them right this time since we're changing them...

This comment has been minimized.

@andrsd

andrsd Jun 2, 2016

Contributor

Parameters are actually fine:

trans_ss_check
ss_check_tol
ss_tmin

The variable that stores the norm had err in it, so I will fix that.

@andrsd

andrsd Jun 2, 2016

Contributor

Parameters are actually fine:

trans_ss_check
ss_check_tol
ss_tmin

The variable that stores the norm had err in it, so I will fix that.

This comment has been minimized.

@friedmud

friedmud Jun 2, 2016

Contributor

Nah - those parameters SUCK - why the trans? And why ss? They don't even show up by each other in Peacock.

I can never remember what they are. Why not:

steady_state_detection
steady_state_tolerance
steady_state_start_time
@friedmud

friedmud Jun 2, 2016

Contributor

Nah - those parameters SUCK - why the trans? And why ss? They don't even show up by each other in Peacock.

I can never remember what they are. Why not:

steady_state_detection
steady_state_tolerance
steady_state_start_time

This comment has been minimized.

@friedmud

friedmud Jun 2, 2016

Contributor

However,

That really sounds like a different PR - so it's fine not to do it now.

I'll create an issue for changing the parameter names...

@friedmud

friedmud Jun 2, 2016

Contributor

However,

That really sounds like a different PR - so it's fine not to do it now.

I'll create an issue for changing the parameter names...

This comment has been minimized.

@andrsd

andrsd Jun 3, 2016

Contributor

What I meant was that they do not have error in them. In that regard they are fine ;-)

@andrsd

andrsd Jun 3, 2016

Contributor

What I meant was that they do not have error in them. In that regard they are fine ;-)

@friedmud

This comment has been minimized.

Show comment
Hide comment
@friedmud

friedmud Jun 2, 2016

Contributor

@andrsd Since we're changing the algorithm here we're going to need to announce this on the lists before we merge it.

I suspect it will make things better for most users - but they should know that it's coming before we just merge it.

Contributor

friedmud commented Jun 2, 2016

@andrsd Since we're changing the algorithm here we're going to need to announce this on the lists before we merge it.

I suspect it will make things better for most users - but they should know that it's coming before we just merge it.

@andrsd

This comment has been minimized.

Show comment
Hide comment
@andrsd

andrsd Jun 3, 2016

Contributor

Pushed a patch that does everything we chatted about (hopefully I did not miss something), except the parameter renaming...

Contributor

andrsd commented Jun 3, 2016

Pushed a patch that does everything we chatted about (hopefully I did not miss something), except the parameter renaming...

Show outdated Hide outdated framework/src/base/NonlinearSystem.C
@@ -163,6 +163,7 @@ NonlinearSystem::NonlinearSystem(FEProblem & fe_problem, const std::string & nam
_preset_nodal_bcs(/*threaded=*/false),
_splits(/*threaded=*/false),
_increment_vec(NULL),
_sln_diff(addVector("sln_diff", false, GHOSTED)),

This comment has been minimized.

@friedmud

friedmud Jun 3, 2016

Contributor

It doesn't need to be GHOSTED... we won't ever be accessing off-processor entries.

@friedmud

friedmud Jun 3, 2016

Contributor

It doesn't need to be GHOSTED... we won't ever be accessing off-processor entries.

Enable users to plot relative norm of the difference in the solution
Refactored the computation of relative norm of the solution difference
- moved into NonlinearSystem. Reusing the function in Transient for both
Picard iterations and check for steady stete.  Improved the memory
allocation - the vector is allocated at the beginning and reused
throughout the simulation. Turning Transient::ss_relerr_norm into a
member variable with a better name.
Added a postprocessor for ploting the relative norm.
Added a test.

Refs #7118
@andrsd

This comment has been minimized.

Show comment
Hide comment
@andrsd

andrsd Jun 3, 2016

Contributor

GHOSTED changed to PARALLEL

Contributor

andrsd commented Jun 3, 2016

GHOSTED changed to PARALLEL

@friedmud

This comment has been minimized.

Show comment
Hide comment
@friedmud

friedmud Jun 3, 2016

Contributor

Thanks @andrsd !

Contributor

friedmud commented Jun 3, 2016

Thanks @andrsd !

@friedmud friedmud merged commit 9d3cf69 into idaholab:devel Jun 3, 2016

10 checks passed

PR TBB test:linux-gnu Passed
Details
PR Pthreads test:linux-gnu Passed
Details
PR Test Heavy:linux-gnu Developer needed to activate
Details
PR Test intel:linux-intel Passed
Details
PR app tests:linux-gnu Passed
Details
PR modules debug:linux-gnu Passed
Details
PR pre check:linux-gnu Passed
Details
PR test debug:linux-gnu Passed
Details
Valgrind Heavy Modules:linux-valgrind Developer needed to activate
Details
Valgrind Heavy:linux-valgrind Developer needed to activate
Details

@andrsd andrsd deleted the andrsd:7118 branch Nov 1, 2016

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