JuliaVTK / ReadVTK.jl

Julia package for reading VTK XML files
https://juliavtk.github.io/ReadVTK.jl
MIT License
30 stars 9 forks source link

Regarding OpenFOAM VTK Files #20

Closed Matthew-Whisenant closed 1 year ago

Matthew-Whisenant commented 1 year ago

I just wanted to comment on the types of VTK files that ReadVTK.jl can read.

My current workflow is: run OpenFOAM -> Paraview to convert foam to vtu -> ReadVTK to load vtu

This works great for everything that is not a scalar. However, OpenFOAM's vtk writer apparently does not include the "NumberOfComponents" tag if a field is a scalar.

On line 610 of ReadVTK we have:

n_components = parse(Int, attribute(xml_element, "NumberOfComponents", required=true))

This requires that field. However, by replacing this line with:

n_components =
    try
      parse(Int, attribute(xml_element, "NumberOfComponents", required=true))
    catch
      1
    end

Then everything works fine. Scalars loaded and I don't see anything loaded incorrectly.

Perhaps this could be added (probably a better way then what I have done here) and then advertised that this can now read OpenFOAM data. I know the CFD field is increasing using Julia so this might be a boon for this repository.

ranocha commented 1 year ago

I think it is a good idea in general to extend this package. However, I would not like to use exception handling for this case - something like has_attribute(xml_element, "NumberOfComponents") should be better, I think.

You are welcome to prepare a PR with the change (including a test) :slightly_smiling_face:

Matthew-Whisenant commented 1 year ago

That sounds good. I am a PhD candidate in school so I do not have much experience doing PRs and open source collaboration in general. But its a good thing to figure out.

Since I can't reproduce a test with WriteVTK, I assume I can do a PR for the examples repo with my included Paraview vtu?

Is it also correct to assume you mean a test for the 'test' command in the package manager?

sloede commented 1 year ago

But its a good thing to figure out.

Great!

Since I can't reproduce a test with WriteVTK, I assume I can do a PR for the examples repo with my included Paraview vtu?

Yeah, that's a bit tricky. You could also generate a test file with WriteVTK.jl (or copy an existing one) and then remove the NumberOfComponents attribute. Maybe @ranocha has another idea?

Is it also correct to assume you mean a test for the 'test' command in the package manager?

Yes, for example to the runtests.jl file on the,test/ folder.

ranocha commented 1 year ago

Since I can't reproduce a test with WriteVTK, I assume I can do a PR for the examples repo with my included Paraview vtu?

Yeah, that's a bit tricky. You could also generate a test file with WriteVTK.jl (or copy an existing one) and then remove the NumberOfComponents attribute.

Yes, that's basically what I had in mind - write your own file for a very simple case. Using WriteVTK.jl and our dependency LightXML.jl should enable you to create a file that mimics the output you get from your standard workflow.

Matthew-Whisenant commented 1 year ago

So I've been working on this, and unfortunately the ParaView vtu format is just too different from WriteVTK to be a good test case. I had to change a line in get_data in order to get the non-appended binary data read in correctly.

Essentially this line https://github.com/trixi-framework/ReadVTK.jl/blob/8ceeb4f9b556fee6066aa170074533dad9f50a92/src/ReadVTK.jl#L657

works great for WriteVTK output but for ParaView vtu it still has a lot of extra stuff. So I just splittext using "\n" which should be safe because base64 doesn't use "\n" (or at least I'm 70% confident).

If I use WriteVTK, this weird formatting doesn't occur, so I think it would make the most sense to add a "celldata_inline_binary_uncompressed_ParaView.vtu" that is just "celldata_inline_binary_uncompressed.vtu" resaved in ParaView and the test is to confirm that the fields match.

Matthew-Whisenant commented 1 year ago

I have finished everything I wanted to add and the tests. Only thing I'm waiting on is if my example file "celldata_inline_binary_uncompressed_ParaView.vtu" can be added to ReadVTK_examples.

Every test passed except a manual connectivity check which I increased by one to match the new increment. I don't think offset needed incrementing unless I am missing some logic.

I am going ahead and doing the PR and PR for ReadVTK_examples. Apologies if something with the PR goes wrong. Just let me know and I can fix it.

I can also contribute to the documentation regarding the ability to read ParaView files, since it HAS to be inline binary format (again some bug with appended raw indexes for cell types that I couldn't figure out). Maybe I could also add a page about my workflow (ParaView VTK out -> ReadVTK -> WriteVTK -> ParaView) as I have seen a couple of posts on Julia Discourse and elsewhere about how to import VTK generated by X software.

I think ParaView would give a great way to really extend the usability of ReadVTK since so much software outputs can be read in by it and output to VTK. The page would discuss how to create a Python trace and get Julia to run the pvserver ParaView command, read in the VTK with ReadVTK and how to convert to write out by WriteVTK. I also understand if it may be beyond the scope of the docs for ReadVTK. I could also make a Julia blog post or something too. Just let me know what y'all think.

sloede commented 1 year ago

So I just splittext using "\n" which should be safe because base64 doesn't use "\n" (or at least I'm 70% confident).

The RFC for Base64 specifically says that non-base64 characters can be allowed to be ignored if the specification referring to the RFC allows it. So it seems to me you could change the line you referenced to remove additional content, i.e., remove \r and \n and all other whitespace from the entire content and not just from beginning and end (as we do right now with strip).

sloede commented 1 year ago

I am going ahead and doing the PR and PR for ReadVTK_examples. Apologies if something with the PR goes wrong. Just let me know and I can fix it.

Yes please! That will it make easier to iterate over the concrete proposal 👍 Please note that Easter vacation season is coming up, so it might take some time for everything to be reviewed.

Matthew-Whisenant commented 1 year ago

So I just splittext using "\n" which should be safe because base64 doesn't use "\n" (or at least I'm 70% confident).

The RFC for Base64 specifically says that non-base64 characters can be allowed to be ignored if the specification referring to the RFC allows it. So it seems to me you could change the line you referenced to remove additional content, i.e., remove \r and \n and all other whitespace from the entire content and not just from beginning and end (as we do right now with strip).

Yes I have done the following:

raw = base64decode(split(strip(content(data_array.data_array)), "\n")[1])

I am going ahead and doing the PR and PR for ReadVTK_examples. Apologies if something with the PR goes wrong. Just let me know and I can fix it.

Yes please! That will it make easier to iterate over the concrete proposal 👍 Please note that Easter vacation season is coming up, so it might take some time for everything to be reviewed.

No problem.