Add prepare_for_use call after VTK read.

[ataber]

To fix this issue

[roystgnr]

Thanks, and sorry for taking so long to get to this!

The current design (where a read() must be explicitly followed by a prepare_for_use() before the mesh is completely read) is certainly questionable, but it was a deliberate design decision, not a bug, so I want to chat with the other developers about how to fix it.

And then if we do want to do a prepare_for_use() internally on users' behalf, then we ought to start doing that for AbaqusIO, CheckpointIO, ExodusII_IO, GmshIO, MatlabIO, NameBasedIO, NemesisIO, OFFIO, TetGenIO, UCDIO, UNVIO, and XdrIO too. If we just try to be nice in one case (like someone else was in GMVIO, it looks like!) then we'll have other formats' users in the same boat later on.

Pinging @jwpeterson, @pbauman, @dknez, @friedmud - is this what we want? Obviously my vote is "yes"; I did the same thing when creating UnstructuredMesh::read(). I can imagine people having a desire for more efficiency, but even if that flexibility is more efficient some times (e.g. a mesh modifier deleting elements based on centroid location wouldn't care if their neighbor links were established first) it seems dangerous (e.g. a modifier deleting elements based on neighboring subdomains or domain boundary would silently produce incorrect results if neighbor links weren't established first).

[dknez]

Calling prepare_for_use internally seems like a good idea to me. I've been caught by this before too.

[pbauman]

I vote "yes" as well. I think if the lack of flexibility becomes as issue, I would think we could introduce a functor interface to allow the user to modify the mesh before prepare_for_use().

[permcody]

From an end-user point of view, I can see the desire to add this internally to prevent common errors. However, in MOOSE our Action system completely takes care of this under all scenarios (reading in only, modifying after read-in, modifying multiple times before use, etc). We've got this down to the minimum number of prepare_for_use() calls with the existing API design. Doing this may prevent errors for the vast majority of cases, but it might be worth understanding speed impacts for those working with really large meshes first. We know that the setup phase of the problem can be really expensive. That being said, perhaps the onus is on us to prove that this might impact speed...

…

On Mon, May 7, 2018 at 7:53 AM Paul T. Bauman ***@***.***> wrote: I vote "yes" as well. I think if the lack of flexibility becomes as issue, I would think we could introduce a functor interface to allow the user to modify the mesh before prepare_for_use(). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1681 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC5XIPgsmj7T0vg-2YZMz24WFuxJixPBks5twFHbgaJpZM4TzcTq> .

[jwpeterson]

I agree that in 90%+ of cases, one wants the mesh to be "prepared" automatically on read. A couple of other reasons we haven't done this in the past are:

1. It does parallel communication (which the user may want to postpone until a later time in the interest of overlapping computation and communication).
2. It renumbers nodes and elements by default, so if there was some information the user wished to preserve about the original numbering of the mesh, that becomes more difficult.
3. It passes default arguments to prepare_for_use() which the user may wish to override (this is partially solved now by the addition of the MeshBase::allow_renumbering() interface)
4. For DistributedMesh, remote elements are deleted on each processor.

I'd be in favor of prepare_for_use() by default, we would need to provide a way, either a MeshInput level flag or by adding default arguments to FooIO::read(), similar to what is currently done in MeshBase::read(), in order for it to be disabled...

[jwpeterson]

@ataber I sent you an invite to join the libMesh "Associates" Team. That just allows your libmesh PRs to begin CI testing automatically.

[roystgnr]

I'd be in favor of prepare_for_use() by default, we would need to provide a way, either a MeshInput level flag or by adding default arguments to FooIO::read(), similar to what is currently done in MeshBase::read(), in order for it to be disabled...

This sounds good to me, if @permcody is happy with it too. Then most users leave out "skip_prepare" and get the most user-friendly behavior, but MOOSE apps can manually avoid redundancy with a small change at the framework level.

[permcody]

This sounds reasonable to me.

[ataber]

As a novice user of this package, I can see why prepare_for_use isn't called automatically, but I did lose a couple hours tracking down the errors. I'm fine with either approach discussed above, but if the code does stay the same there should be some more visible documentation about the necessity of calling prepare_for_use after reading. Thanks!

[jwpeterson]

This PR has been marked "do not merge" since we are no longer accepting PRs into the master branch. All new PRs should be made on the devel branch instead. Once this PR's target branch has been updated to devel, the "do not merge" label will be removed.