search

JuliaVTK / ReadVTK.jl

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

ParaView read ability #31

Closed Matthew-Whisenant closed 1 year ago

Matthew-Whisenant commented 1 year ago

See #28

Edit: for some reason I expected the other PR commits to auto-resolve and not be included in this PR. Maybe after they other PR is accepted?

sloede commented 1 year ago

for some reason I expected the other PR commits to auto-resolve and not be included in this PR. Maybe after they other PR is accepted?

I think there might be some conflicts you'll need to resolve. However, since this PR seems to depend on #30, I will wait with a review until #31 is merged (which imho can be very soon)

Matthew-Whisenant commented 1 year ago

Everything should be in order now. Now that I know how the formatter works and is intergraded into vscode it seems like a great addition.

Again, PR depends on https://github.com/JuliaVTK/ReadVTK_examples/pull/1

All concerns should be address from previous PR https://github.com/JuliaVTK/ReadVTK.jl/pull/28

  • It seems reasonable for me that the user should call a conversion function explicitly if they want to access the cell information in a WriteVTK.jl-compatible manner. Or does anyone think this should be the default?

It would make sense if it was default to me, but that's probably for a bigger rebase if/when the exported types from both ReadVTK and WriteVTK are hammered down, such as VTKFile such that both add methods to existing types from VTKBase. At that point, it would also just make sense to have a central VTKIO package that imports both write and read, much like DifferentialEquations.jl and its library of DiffEq packages.

But that's a lot of work and I think the current way is perfectly fine.

Matthew-Whisenant commented 1 year ago

https://github.com/JuliaVTK/ReadVTK.jl/blob/ec581dcfac5ffd54b3fcd1887cc21c8e2a662c72/test/runtests.jl#L5-L10

I suppose this needs to be updated to: 1178cafb12bfac2a6d249f3ef0da9423f6c7202e so that it pulls the latest repo?

sloede commented 1 year ago

https://github.com/JuliaVTK/ReadVTK.jl/blob/ec581dcfac5ffd54b3fcd1887cc21c8e2a662c72/test/runtests.jl#L5-L10

I suppose this needs to be updated to: 1178cafb12bfac2a6d249f3ef0da9423f6c7202e so that it pulls the latest repo?

Yes, I believe so 👍

Matthew-Whisenant commented 1 year ago

Thanks, I've closed #21 and #20 because this was merged.

sloede commented 1 year ago

👍 Thanks! Please feel welcome to further improve the capabilities of ReadVTK.jl any time 🙂