Adding a new function `pvtk_grid` to generate parallel file formats (e.g. .pvtu) files. by fverdugo · Pull Request #90 · JuliaVTK/WriteVTK.jl

fverdugo · 2021-10-21T10:24:35Z

@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.

…teVTK.jl into support_parallel_file_formats

Added minimum tests (to be completed)

Support parallel file formats

jipolanco · 2021-10-21T10:32:21Z

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.

jipolanco · 2021-10-21T11:52:35Z

+(with the exception of file names that are augmented with the
+corresponding part id).
+
+The extra keyword argument `pvtkargs` contains options


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.

jipolanco · 2021-10-21T11:53:40Z

+    pvtkargs::Dict
+    xdoc::XMLDocument
+    dataset::DatasetFile
+    path::AbstractString


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

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

jipolanco · 2021-10-21T11:55:15Z

@@ -0,0 +1,322 @@
+
+struct ParallelDatasetFile <: VTKFile
+    pvtkargs::Dict


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

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?

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.

jipolanco · 2021-10-21T11:59:28Z

+end
+
+
+const string_to_VTKDataType = Dict("Int8"=>Int8,


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.

jipolanco · 2021-10-21T12:00:41Z

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


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

fverdugo · 2021-10-21T13:16:04Z

@amartinhuertas I'll try to address the comments

amartinhuertas · 2021-10-21T13:33:40Z

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

codecov-commenter · 2021-10-21T15:55:48Z

Codecov Report

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

@@            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 · 2021-10-21T15:57:25Z

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

jipolanco · 2021-10-21T16:14:41Z

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

amartinhuertas and others added 17 commits October 17, 2021 23:40

Working on a proposal to support parallel file formats

9c2b314

A more definite (still partial though) sketch for pvtk_grid

ab9f3cb

Update proposal_pvtk_grid.jl

5e97d2f

Implemented new high level API that mirrors the vtk_grid sequential one

32b1ef4

Merge branch 'support_parallel_file_formats' of github.com:gridap/Wri…

1433f09

…teVTK.jl into support_parallel_file_formats

Moving proposal to something more definite

4393d2f

Moving (draft) pvtk_grid proposal to source directory

50558a9

Added minimum tests (to be completed)

Moving file to gridtypes folder

31fa4ce

Deleting VTKLayout and other improvements.

b2b6a09

Merge pull request #1 from gridap/support_parallel_file_formats

15773c2

Support parallel file formats

Update README.md

eaa7b73

Some typos in README

435203a

Minor

d014673

Make names in pvtk consistent with actual filenames

d2ac5bd

More tests in pvtk_grid

acfed0d

Use a function to generate serial names in pvtk_grid

0100f17

Update README.md

35f5fe5

jipolanco reviewed Oct 21, 2021

View reviewed changes

fverdugo added 2 commits October 21, 2021 17:41

Refactoring in pvtk_grid to meet PR review

abd9f3c

More exhaustive testing

ec2f401

jipolanco merged commit cc6d198 into JuliaVTK:master Oct 21, 2021

amartinhuertas mentioned this pull request Oct 21, 2021

Pending work VTK parallel file formats #91

Open

3 tasks

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

		struct ParallelDatasetFile <: VTKFile
		pvtkargs::Dict

		end


		const string_to_VTKDataType = Dict("Int8"=>Int8,

Conversation

fverdugo commented Oct 21, 2021

Uh oh!

jipolanco commented Oct 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fverdugo commented Oct 21, 2021

Uh oh!

amartinhuertas commented Oct 21, 2021

Uh oh!

codecov-commenter commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fverdugo commented Oct 21, 2021

Uh oh!

jipolanco commented Oct 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Oct 21, 2021 •

edited

Loading