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

Adding a new function pvtk_grid to generate parallel file formats (e.g. .pvtu) files. #90

Merged
merged 19 commits into from
Oct 21, 2021

Conversation

fverdugo
Copy link
Contributor

Hi @jipolanco,

@amartinhuertas and I have implemented a new function pvtk_grid to generate parallel file formats.

In particular, we need this to export simulation results in pvtu format from our parallel code GridapDistributed.jl. As far as we could tell, this was not available before in WriteVTK.

We believe that this is a feature that many users can benefit from and thus we happily put some effort in preparing this PR. We have also documented the usage of this function in the README.md file.

We hope you find this PR useful.

@jipolanco
Copy link
Member

Hi @fverdugo and @amartinhuertas,

This is definitely a great addition to WriteVTK! Thanks a lot for the PR and for all the effort.

I will take a look at the PR in more detail and let you know if I have any comments, and I hope that it can be merged soon.

README.md Outdated
(with the exception of file names that are augmented with the
corresponding part id).

The extra keyword argument `pvtkargs` contains options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make these regular keyword arguments? As in:

pvtk_grid(args...; part, nparts, ismain = (part == 1), ghost_level = 0, kwargs...)

Note that part and nparts are mandatory here, while ismain and ghost_level are optional. I think it would be simpler for users, and would probably also simplify the parsing.

pvtkargs::Dict
xdoc::XMLDocument
dataset::DatasetFile
path::AbstractString
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a String for type stability. Alternatively, the type should be included as a parameter of ParallelDatasetFile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine to convert to String, that's what much of the Julia base code does.

@@ -0,0 +1,322 @@

struct ParallelDatasetFile <: VTKFile
pvtkargs::Dict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dict is not a concrete type. Maybe use a NamedTuple instead, and to make things type stable, the type can be included as a parameter of ParallelDatasetFile:

struct ParallelDatasetFile{Args <: NamedTuple} <: VTKFile
    pvtkargs::Args

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessary to specialize on this by putting it as a type. A typed dict (Dict{Symbol,String} etc) should be enough IMO. And honestly, compared to the performance of writing the file I can hardly imagine it is a problem to have the field as abstract either, or?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that, in this case, leaving the field as abstract won't make a noticeable difference in performance. My concern was more about consistency with the rest of the codebase, where things are concrete whenever possible.

In particular, I would say that the ParallelDatasetFile type could be written similarly to DatasetFile, where types are strongly enforced (and where each parameter is stored in its own separate field). But I'm OK with leaving things as they are for now.

end


const string_to_VTKDataType = Dict("Int8"=>Int8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be avoided? This is (very) type-unstable. If I understand correctly, this is used to convert a string to a type, which ultimately gets converted back to a string. It would be better to just pass the strings around.

"Float64"=>Float64,
"String"=>String)

"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use regular keyword arguments instead of a Dict as input? (See comment in README.md.)

@fverdugo
Copy link
Contributor Author

@amartinhuertas I'll try to address the comments

@amartinhuertas
Copy link
Contributor

Ok @fverdugo let me know the comments which might be pending to address

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #90 (ec2f401) into master (f9f2429) will increase coverage by 0.81%.
The diff coverage is 98.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   92.32%   93.14%   +0.81%     
==========================================
  Files          15       16       +1     
  Lines         691      788      +97     
==========================================
+ Hits          638      734      +96     
- Misses         53       54       +1     
Impacted Files Coverage Δ
src/WriteVTK.jl 84.21% <ø> (ø)
src/gridtypes/pvtk_grid.jl 98.96% <98.96%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9f2429...ec2f401. Read the comment docs.

@fverdugo
Copy link
Contributor Author

@jipolanco @fredrikekre I have addressed your comments. I have also tried to simplify a bit the code.

@jipolanco
Copy link
Member

Looks good to me! Thanks again for the PR. I'll try to push a new version soon.

@jipolanco jipolanco merged commit cc6d198 into JuliaVTK:master Oct 21, 2021
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.

5 participants