Add a function to retrieve the current last mesh generator #15125
Conversation
|
Job Precheck on 73766cd wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
YaqiWang
force-pushed
the
get_final_mesh_generator
branch
2 times, most recently
from
b40ee1f
to
8cb44a5
Compare
|
Job Documentation on 78fca05 wanted to post the following: View the site here This comment will be updated on new commits. |
|
This is ready for review or merge. |
| mooseError("Cannot append a mesh generator that does not take input mesh generators"); | ||
|
|
||
| _moose_object_pars.set<MeshGeneratorName>("input") = _app.getCurrentFinalMeshGenerator(); | ||
| _app.addMeshGenerator(_type, _name, _moose_object_pars); |
There was a problem hiding this comment.
Is this your use case, or do you expect others? The reason I ask is I don't see a reason to expose the name of the last generator at all if your use case is to add to the "end". A better design would be to add a new method called "appendMeshGenerators" that just does the appending internally. Exposing the end as you've done in this PR has issues. As you've already stated, it doesn't quite work correctly if the user also explicitly sets the final generator.
There was a problem hiding this comment.
You are probably right. I do not need to expose this but rather create appendMeshGenerator function. Let me rewrite this PR.
framework/src/base/MooseApp.C
Outdated
|
|
||
| std::string final_generator_name; | ||
| if (ordered_generators.size()) | ||
| final_generator_name = ordered_generators.back().back()->name(); |
There was a problem hiding this comment.
Rather than duplicate this logic, perhaps you could factor out this piece from the execute method and just call it from there? However, see my other comment below as I don't think you should expose this API at all. Especially if it's not always correct.
YaqiWang
force-pushed
the
get_final_mesh_generator
branch
from
8cb44a5
to
055280e
Compare
|
@permcody Your comments are addressed. I create a function |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I initially tried to delete _mesh_generators here after _ordered_generators is created. But got errors. So I end it up putting the clear at Line 1758.
There was a problem hiding this comment.
Shared pointers are easy to copy. You just do normal assignment and you have a copy. As long as somebody is holding a pointer to the underlying object, it will not be deleted. So I don't see how a clear of one vector would cause errors unless it holds the only pointer to an object. What was the error?
There was a problem hiding this comment.
I think doing _mesh_generators.clear() after _ordered_generators is created seems deleted all mesh generators. All mesh generator tests failed. Once I moved the clear later after all mesh generators are executed, every test works fine. I did not look into it further. But seems inside DependencyResolver, shared pointer copying is not doing the right thing. I have no much experience with shared pointer though.
There was a problem hiding this comment.
Btw, the diff seems a lot, but I actually did not change much in the code.
There was a problem hiding this comment.
The _mesh_generators vector is the "warehouse" for the generators. We don't have a second copy of those anywhere. They must not be cleared before execution.
There was a problem hiding this comment.
The issue might be that _mesh_generators are accessed through public APIs. I bet there are some assumptions about getting access to the generators externally, which are probably where your errors come from.
|
@cody, if you do not care if the |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The issue might be that _mesh_generators are accessed through public APIs. I bet there are some assumptions about getting access to the generators externally, which are probably where your errors come from.
|
@permcody can this be merged? Thanks. |
|
Looks like we forgot to drop the declaration for that one method. I'll merge this one complete. |
|
Oh, my bad. Sorry. |
Close #15121.