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

Output after every AMR step #530

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tradowsk
Copy link
Collaborator

First attempt at a refactoring of SteadyMeshAdaptiveSolver so that the visualization output does not get overwritten on every AMR step.

The new input flag vis-options/output_every_amr is set to false by default. Setting it to true will output files in the format: prefix.amrX.format (e.g. test.amr0.xdr,test.amr1_mesh.xda etc.). If you would prefer a different convention, I would be happy to change it.

I also moved the 'meat' of the AMR loop in SteadyMeshAdaptiveSolver::solve() to a helper function so that it can be called before and after the AMR has been performed. For example, for max_refinement_steps = '1', you would get an output (QoI value, error estimates, vis, etc.) before the AMR is done and after. I find this more intuitive than the previous design, where max_refinement_steps = '1' wouldn't actually refine/coarsen the mesh.

I haven't done anything in UnsteadyMeshAdaptiveSolver yet, so I just put a warning message in there to display if the user sets output_every_amr = 'true'.

Like I said, this is a first attempt that meets my specific needs, but I'm open to suggestions.

@tradowsk
Copy link
Collaborator Author

Looks like I need to update some of the tests, but I'll wait to do that until we finalize the output design

@@ -71,7 +71,7 @@ namespace GRINS
if( comm.rank() == 0 )
{
std::ofstream output;
output.open( _file_prefix+".dat" );
output.open( _file_prefix+".dat",std::ofstream::app );
Copy link
Member

Choose a reason for hiding this comment

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

Why force append?

Copy link
Collaborator Author

@tradowsk tradowsk Jan 17, 2018

Choose a reason for hiding this comment

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

I guess I can add an additional bool append = false parameter so it can be toggled.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I guess it's not unreasonable to just append QoI values after each AMR step. And if you're not doing AMR, it won't matter. OK, I'm fine with just having it append.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only potential problem I can see is that if the file already exists from a previous run and we append to it, it might get confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I thought about that, but if we're not appending, we're overwriting. Six in one, half dozen the other IMO, so instead of adding a bunch of extra code to worry about the case, let's just carefully document in the master_example_inputfile.in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good

libMesh::AdjointRefinementEstimator* adjoint_ref_error_estimator = libMesh::cast_ptr<libMesh::AdjointRefinementEstimator*>( context.error_estimator.get() );
std::cout<<"The error estimate for QoI("<<i<<") is: "<<adjoint_ref_error_estimator->get_global_QoI_error_estimate(i)<<std::endl;
}
if (context.output_every_amr)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a bool, maybe an integer to output every n refinement steps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess that would make sense, especially in the context of the UnsteadyMeshAdaptiveSolver

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was actually thinking about unsteady case, but yeah, for steady case, let's be able to reuse that code and if we're just and if the user put something > 1, we can flash a warning and switch it back to 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, not quite sure what you're saying here...

Copy link
Member

Choose a reason for hiding this comment

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

What my stream-of-consciousness comments don't make sense?! :P Let's use integer for steady case, but if the user puts anything other than 0 or 1, we print a warning and change it back to 0 or 1 (whatever makes sense).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hadn't had my morning tea yet, that makes more sense haha


this->dump_visualization( equation_system, filename, 0.0 );

return;
Copy link
Member

Choose a reason for hiding this comment

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

This was bad style that I'd introduced long ago, but haven't removed from everywhere, but let's not introduce more. For void functions, we just let the closing brace be the end and don't explicitly call return.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, too much copy-pasta...

<< "==========================================================" << std::endl
<< "Post-AMR Output " << std::endl
<< "==========================================================" << std::endl;
this->amr_helper(context,_mesh_adaptivity_options.max_refinement_steps(),false);
Copy link
Member

Choose a reason for hiding this comment

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

Just checking that I'm reading this correclty: this will ensure we do one final solve after the last refinement step? This was actually missing before, we did an AMR step, but didn't solve to update those new dofs, they were just constrained from the previous solution (which is not entirely egregious, but not as accurate).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right: previously a 1-step AMR run wouldn't actually refine the mesh at all, just calculate the errors and output the original mesh if requested. Intuitively, I would assume that a 10-step AMR run would refine the mesh 10 times and give me 11 error estimates and QoI values (from the original mesh and after the 10 refinements)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do need to make a small modification here so that I can get my QoI error estimates without doing a refinement

@pbauman
Copy link
Member

pbauman commented Jan 17, 2018

I think I'd prefer test.amr.0.xdr etc. for the naming scheme (just the extra . before the integer).

@tradowsk
Copy link
Collaborator Author

I think I'd prefer test.amr.0.xdr etc. for the naming scheme (just the extra . before the integer).

I wonder how this scheme will work if we have to append the timestep index as well in the unsteady case. test.amr.0.0.xdr?

@tradowsk tradowsk force-pushed the refactor-solver branch 2 times, most recently from a0aaa50 to db05479 Compare January 17, 2018 15:46
@pbauman
Copy link
Member

pbauman commented Jan 17, 2018

I wonder how this scheme will work if we have to append the timestep index as well in the unsteady case. test.amr.0.0.xdr?

Lol. What about when we get around to adding CheckpointIO?! test.amr.0.0.0.datYeah, that'll be a horror show. We'll want to update the unsteady naming to be something like test.amr.0.ts.0.

@tradowsk
Copy link
Collaborator Author

Alright, I think I have a decent design here with just some minor tweaks.

I'm going to hold off on those for a while so I can get some runs in and nail down a good test case.

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.

2 participants