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

Support for StructuredGrid files #18

Merged
merged 91 commits into from Mar 9, 2023
Merged

Support for StructuredGrid files #18

merged 91 commits into from Mar 9, 2023

Conversation

boriskaus
Copy link
Contributor

@boriskaus boriskaus commented Mar 6, 2023

This PR adds support for StructuredGrid (*.vts) files as well as their parallel counterparts PStructuredGrid (*.pvts)

boriskaus and others added 30 commits February 24, 2023 09:49
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
@boriskaus
Copy link
Contributor Author

So this mistake sometimes occurs in CI and sometimes not, even when we cleanup the files altogether. I don't like this at all...

@sloede
Copy link
Member

sloede commented Mar 8, 2023

So this mistake sometimes occurs in CI and sometimes not, even when we cleanup the files altogether. I don't like this at all...

Can you try fixing it? It seems to me like a very specific error, not related to floating point truncation.

@boriskaus
Copy link
Contributor Author

I have no clue where to start. Seems that the files written to disc sometimes have a different size. Not having a local machine that reproduces this error does not help..

@boriskaus
Copy link
Contributor Author

boriskaus commented Mar 8, 2023

Note that the main branch is also failing now
with what feels to be a fairly similar problem.

@boriskaus
Copy link
Contributor Author

@sloede: Whereas CI is fine now, the error remains weird and occurs when reading compressed data back from a file here:

raw = data_array.vtk_file.appended_data
HeaderType = header_type(data_array.vtk_file)
if is_compressed(data_array)
   # If data is stored compressed, the first four integers of type `header_type` are the header and
   # the fourth value contains the number of bytes to read
   first = data_array.offset + 1
   last = data_array.offset + 4 * sizeof(HeaderType)
   header = Int.(reinterpret(HeaderType, raw[first:last]))
   n_bytes = header[4]

   first = data_array.offset + 4 * sizeof(HeaderType) + 1
   last = first + n_bytes - 1
   uncompressed = transcode(ZlibDecompressor, raw[first:last])
else
   ...
end

in some cases, the length of the raw vector is shorter than last as indicated in the header. The CI failure in the main branch seems to be caused by the same issue.
In my last commit, I simply added an error message when this occurs, but it doesn't cure the cause of it.

Are there perhaps changes in the upstream packages that could cause this?

@sloede
Copy link
Member

sloede commented Mar 9, 2023

in some cases, the length of the raw vector is shorter than last as indicated in the header. The CI failure in the main branch seems to be caused by the same issue. In my last commit, I simply added an error message when this occurs, but it doesn't cure the cause of it.

Thanks for investigating the issue. Do you know why the error now disappeared? And did it only occur in cases where the file in question was generated on-the-fly or also in cases where an existing vtk file is used?

Are there perhaps changes in the upstream packages that could cause this?

I believe we had a similar issue before - @andrewwinters5000 do you still remember where and when this occurred? I do not remember what we did back then to resolve it, but I recall that we had a similar issue of files being "truncated" by the zip/unzip mechanism.

src/ReadVTK.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@ranocha
Copy link
Member

ranocha commented Mar 9, 2023

in some cases, the length of the raw vector is shorter than last as indicated in the header. The CI failure in the main branch seems to be caused by the same issue. In my last commit, I simply added an error message when this occurs, but it doesn't cure the cause of it.

Thanks for investigating the issue. Do you know why the error now disappeared? And did it only occur in cases where the file in question was generated on-the-fly or also in cases where an existing vtk file is used?

Are there perhaps changes in the upstream packages that could cause this?

I believe we had a similar issue before - @andrewwinters5000 do you still remember where and when this occurred? I do not remember what we did back then to resolve it, but I recall that we had a similar issue of files being "truncated" by the zip/unzip mechanism.

I also vaguely remember some spurious issues like these. If I remember correctly, they had some stochastic part, i.e., they were not triggered every time - maybe depending on the GitHub runners etc.? Some parts may have been specific to Windows or so...

@boriskaus
Copy link
Contributor Author

boriskaus commented Mar 9, 2023

in some cases, the length of the raw vector is shorter than last as indicated in the header. The CI failure in the main branch seems to be caused by the same issue. In my last commit, I simply added an error message when this occurs, but it doesn't cure the cause of it.

Thanks for investigating the issue. Do you know why the error now disappeared?

No, if you look at the various CI's of yesterday, it sometimes appears and sometimes not (on different systems)

And did it only occur in cases where the file in question was generated on-the-fly or also in cases where an existing vtk file is used?

I believe it is only with cases where the file was generated on-the-fly.

Are there perhaps changes in the upstream packages that could cause this?

I believe we had a similar issue before - @andrewwinters5000 do you still remember where and when this occurred? I do not remember what we did back then to resolve it, but I recall that we had a similar issue of files being "truncated" by the zip/unzip mechanism.

@sloede
Copy link
Member

sloede commented Mar 9, 2023

OK. Since the error seems to have disappeared, once the last remaining issues are resolved, we can merge this.

boriskaus and others added 4 commits March 9, 2023 14:27
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
@boriskaus
Copy link
Contributor Author

so we now have 1 failed CI and 2 that passed, with the only thing that changed being:

@show data_array data_array.vtk_file.xml_file

and removing:

# Start with a clean environment: remove example file directory if it exists
isdir(TEST_EXAMPLES_DIR) && rm(TEST_EXAMPLES_DIR, recursive=true)
mkpath(TEST_EXAMPLES_DIR)

It seems to be rather stochastic and I have no idea where to look.
I guess we can probably merge (and increment the version number) and see if it keeps occurring.

@ranocha
Copy link
Member

ranocha commented Mar 9, 2023

It seems to be rather stochastic and I have no idea where to look.

That's also what I remember from similar issues earlier - it's stochastic and when it fails, it's on Windows.

I guess we can probably merge (and increment the version number) and see if it keeps occurring.

Sounds good to me. Thanks a lot for your contributions!

src/ReadVTK.jl Outdated Show resolved Hide resolved
src/ReadVTK.jl Outdated Show resolved Hide resolved
@sloede sloede enabled auto-merge (squash) March 9, 2023 20:44
@sloede sloede disabled auto-merge March 9, 2023 21:13
@sloede sloede merged commit 501b06f into JuliaVTK:main Mar 9, 2023
8 of 9 checks passed
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.

None yet

4 participants