The WriteVTK wrapper: Multiblockfiles and VTKFileCollection

lijas commented 3 days ago

Before the introduction of the WriteVTK wrapper in #692 , it was easier to work with multiblock files and paraview collection files (to save multiple time steps). From one of my packages:

pvd = paraview_collection(filename)
...
vtmfile = WriteVTK.vtk_multiblock(filename)
for part in parts
    vtkfile = vtk_grid(partname, part) #e.g special output for IGA or Immersed grids
    WriteVTK.multiblock_add_block(vtmfile, vtkfile)
    ...
end
vtk_save(vtmfile)
pvd[state.t] = vtmfile

This has changed now:

The creation of paraview collection files has changed. Now they require a grid/dofhadler as an argument for the constructor (VTKFileCollection(name, grid_or_dh)), which is not always known at the construction of the paraview collection. We should either make it possible to have grid_or_dh===nothing or remove it completely (depending on if we only want to have a thin wrapper or extend it with our own functionality).
We dont have any wrappers for multiblockfiles atm.

KnutAM commented 3 days ago

Adding 1.0 since removing grid_or_dh from VTKFileCollection will be breaking, since this will make it required to include when calling addstep!.

Also pointing the comments where the multiblock was discussed in #692 since the discussion is so long there.

KnutAM commented 2 days ago

Just thinking out loud for some options

1) Introduce wrappers for the collection types

using Ferrite
pvd = VTMFileCollection(filename)
...
addstep!(pvd, t) do vtm # ::VTMFile
    for part in parts
        VTKFile(vtm, partname, part.grid) do vtk 
            write_solution(vtk, dh, u)
            write_...
        end
    end
end

# Without `addstep!`
pvd = VTMFileCollection(filename)
...
vtm = VTMFile("my_vtm_file")
for part in parts
    VTKFile(vtm, partname, part.grid) do vtk 
        write_solution(vtk, dh, u)
        write_...
    end
end
pvd[state.t] = vtm

2) Remove `VTKFileCollection` completely

Users who wants anything more than VTKFile must use WriteVTK explicitly Standard usage with collection

using Ferrite, WriteVTK
pvd = paraview_collection(filename)
...
VTKFile(name * "_$step$", grid_or_dh; kwargs...) do vtk 
    write_solution(...)
    ...
    pvd[t] = vtk
end
...
vtk_save(pvd)

Usage with vtm files

using Ferrite, WriteVTK
pvd = paraview_collection(filename)
...
vtmfile = WriteVTK.vtk_multiblock(filename * "_$step")
for part in parts
    VTKFile(vtmfile, partname, part.grid_or_dh) do vtk 
        write_solution(vtk, ...)
    end
end
pvd[state.t] = vtmfile # I think vtk_save(vtmfile) is done in this call
...
vtk_save(pvd)

I kindof liked the fact that you don't have to name the individual vtu files with addstep. This could perhaps be made possible by supporting VTKFile(pvd, t, grid_or_dh) similar to VTKFile(vtm, partname, grid_or_dh)?

Looking at the WriteVTK docs, I think we would need to have a wrapper for .pvtk files too, but that is a later concern, and this would be very thin and local to where you write, i.e. users should call PVTKFile rather than pvtk_grid.

Any other suggestions?

termi-official commented 2 days ago

2. We dont have any wrappers for multiblockfiles atm.

Independent of the result in the discussion on this specific format, I think we should provide a guide on how to extend the writers.

Looking at the WriteVTK docs, I think we would need to have a wrapper for .pvtk files too, but that is a later concern, and this would be very thin and local to where you write, i.e. users should call PVTKFile rather than pvtk_grid.

That would be a FerriteDistributed.jl issue I think.

Just thinking out loud for some options

1) Introduce wrappers for the collection types
using Ferrite
pvd = VTMFileCollection(filename)
...
addstep!(pvd, t) do vtm # ::VTMFile
    for part in parts
        VTKFile(vtm, partname, part.grid) do vtk 
            write_solution(vtk, dh, u)
            write_...
        end
    end
end

# Without `addstep!`
pvd = VTMFileCollection(filename)
...
vtm = VTMFile("my_vtm_file")
for part in parts
    VTKFile(vtm, partname, part.grid) do vtk 
        write_solution(vtk, dh, u)
        write_...
    end
end
pvd[state.t] = vtm
This does look different from the current design. I thought we agree on using VTKFileCollection/VTKFile/... such that we have an abstraction over all the formats? Can't we use addstep! in combination with some addblock! or similar here?

KnutAM commented 2 days ago

This does look different from the current design. I thought we agree on using VTKFileCollection/VTKFile/... such that we have an abstraction over all the formats? Can't we use addstep! in combination with some addblock! or similar here?

Yes, we could do addblock!(vtm, part.grid) do vtk, but not sure if this generalizes to other file formats the same adding time steps does.

Right! we should then have VTKFileCollection return a VTMFile by providing an option upon construction!

However, a VTMFile is just a collection of vtk files, so it doesn't make sense to wrap this in the VTKFile type if that is what you suggest, but maybe I misunderstand? My understanding was rather that VTKFile should (via #828 ) should support different cases from Ferrite, such as DG or IGA, but always be a vtu file.

That would be a FerriteDistributed.jl issue I think.

yes - just wanted to keep that in mind :)

termi-official commented 1 day ago

However, a VTMFile is just a collection of vtk files, so it doesn't make sense to wrap this in the VTKFile type if that is what you suggest, but maybe I misunderstand?

Why? A multiblock is literally a type of VTK file. It is also modeled this way in WriteVTK (https://juliavtk.github.io/WriteVTK.jl/stable/API/#WriteVTK.MultiblockFile).

KnutAM commented 1 day ago

Maybe I got it wrong, but I read "Multiblock files (.vtm) are XML VTK files that can point to multiple other VTK files.", and interpreted this as a collection of vtk files similar to pvd. Especially since it is listed under "Metadata formats".

And the point of not putting a multiblock in the VTKFile type is that you cannot do e.g. write_solution(vtk::VTKFile, ...) in that case.

So my logic is that VTKFile corresponds to vtk_grid, but by having it as our own type, we can extend it to work for different cases on the Ferrite side. One option would be to provide our own wrappers also for the other "Metadata formats", but it might be easier to use these formats directly, by just overloading them for our VTKFile type (hence suggestion 2)

termi-official commented 1 day ago

Maybe I got it wrong, but I read "Multiblock files (.vtm) are XML VTK files that can point to multiple other VTK files.", and interpreted this as a collection of vtk files similar to pvd. Especially since it is listed under "Metadata formats".

Correct. I have said nothing else. :)

And the point of not putting a multiblock in the VTKFile type is that you cannot do e.g. write_solution(vtk::VTKFile, ...) in that case.

Not sure if this is the right interface anyway. It should be write_solution(::DatasetFile,...), because it contains most of the VTK data formats, right? Or what is the intent of having a writer on the abstract type?

KnutAM commented 1 day ago

Yes, Ferrite.VTKFile (not to be confused with the abstract WriteVTK.VTKFile) is a wrapper for WriteVTK.DatasetFile. The purpose of wrapping it was to allow us to dispatch in a better way, and to allow addition additional data as needed.

supertypes(WriteVTK.MultiblockFile) are (WriteVTK.MultiblockFile, WriteVTK.VTKFile, Any) and supertypes(WriteVTK.VTKBlock) are (WriteVTK.VTKBlock, Any).

termi-official commented 1 day ago

Yes, Ferrite.VTKFile (not to be confused with the abstract WriteVTK.VTKFile) is a wrapper for WriteVTK.DatasetFile. The purpose of wrapping it was to allow us to dispatch in a better way, and to allow addition additional data as needed.

I see. We should change the name then, as I think this is confusing on two levels. I think the naming here is a bit unlucky, because our VTK file is not really a wrapper for VTK files but for VTU files, right? Furthermore we can introduce some supertype for vtk data files. This would also fit with the proposal in this discussion, that we can add some VTMFile which "groups" other VTK file types. What do you think?

KnutAM commented 1 day ago

I see. We should change the name then, as I think this is confusing on two levels. I think the naming here is a bit unlucky, because our VTK file is not really a wrapper for VTK files but for VTU files, right?

It wraps WriteVTK.DatasetFile which supports structured (.vts, .vtr, and .vti) and unstructured .vtu, but we only deal with .vtu files for Ferrite. But as you say, not the collection files, .vtm, .vtm, or any of the pxxx files (e.g. pvtu).

I think changing the name might be a good idea, seeing this confusion! While VTKGrid makes sense matching the vtk_grid function, but that could be confusing as it isn't an Ferrite.AbstractGrid. Perhaps VTKGridFile to highlight that it is a file representing a grid (structured or unstructured), but not a collection file?

Furthermore we can introduce some supertype for vtk data files.

I don't see what the common interface would be here? The other vtk file collect multiple Ferrite.VTKFiles or other collection files, whereas Ferrite.VTKFile really stores the data.

IMO, a supertype should rather have subtypes that support the write_xyz methods. This would apply to other export formats.

KnutAM commented 1 day ago

And the more we discuss this, the clearer it is to me that it was probably a mistake to introduce the VTKFileCollection as it isn't flexible enough. It is probably better to revert this, what do you say @lijas and @fredrikekre ?

termi-official commented 1 day ago

[...] I think changing the name might be a good idea, seeing this confusion! While VTKGrid makes sense matching the vtk_grid function, but that could be confusing as it isn't an Ferrite.AbstractGrid. Perhaps VTKGridFile to highlight that it is a file representing a grid (structured or unstructured), but not a collection file?

:+1:

Furthermore we can introduce some supertype for vtk data files.

I don't see what the common interface would be here? The other vtk file collect multiple Ferrite.VTKFiles or other collection files, whereas Ferrite.VTKFile really stores the data.

IMO, a supertype should rather have subtypes that support the write_xyz methods. This would apply to other export formats.

Sorry I cannot reconstruct my train of thought how I arrived at the conclusion. Will post if I catch it again.

And the more we discuss this, the clearer it is to me that it was probably a mistake to introduce the VTKFileCollection as it isn't flexible enough. It is probably better to revert this, what do you say @lijas and @fredrikekre ?

Not sure about this. Maybe we should put some thought into making this more flexible?

KnutAM commented 1 day ago

Not sure about this. Maybe we should put some thought into making this more flexible?

I'm not sure how, but happy to get suggestions! This is quite urgent due to 1.0, so learning from the previous mistakes, I think the most important right now is to keep the 0.3.14 features while promising as little as possible - we can always provide something nicer in the future.

For now, I'm fixing the naming first in #1014 - there I also realized now that we had used WriteVTK.VTKFile as the supertype for the element, instead of WriteVTK.DatasetFile, which probably was a key source of the confusion.

lijas commented 1 day ago

So if I understand correctly, the alternatives are

1) remove the Ferrite.VTKFileCollection wrapper (because currently it is a bit too restrictive), and let users handle multiblock files and timestep-files by them self using WriteVTK.paraview_collection and WriteVTK.vtk_multiblock

2) Make the Ferrite.VTKFileColellection-wrapper a bit "thinner" (around WriteVTK.CollectionFile), and also add a thin wrapper for around WriteVTK.VTKBlock.

Maybe 1) is a bit more future proof, where we can decide on an "export" interface in the future?

KnutAM commented 1 day ago

Yes, see my comments in #1015 for how I see that option one could still be quite user-friendly in the future, while keeping the 0.3.14 behavior for now.

Ferrite-FEM / Ferrite.jl