librelist archives

« back to archive

Custom cell data in the vtk exporter

Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2014-09-21 @ 01:12
Hey Carsten et al.

The application that I'm developing using p4est really needs to be able to
write out custom cell data. (Cell centered CFD solution data in this case.)
Currently, as you know, p4est only supports custom point data. At first, as
a work around, I transfered my cell-centered data to each of the octant
corners and wrote it out that way (like step3_write_solution() does
in example/steps/p4est_step3.c) This does work, kind of; but it is much
less than optimal for me and it only works if the octant scale parameter is
less than one such that there are unique corner points/nodes for every
octant.

I have gone ahead and added logic that allows for custom cell data to be
exported to the vtu files. It seems to work like a charm so far but it is
pretty kludgy. The first thing I tried was to simply copy the way that
custom point data is written; this does not work however as there can not
be more than one <CellData Scalars> ... </CellData> sections in the vtu
file (as far as I can tell anyway).

This created a bit of a dilemma for me as the cell data that is already
exported (treeid, level, mpirank) is written in the
p{4,8}est_vtk_write_header() function. As I didn't want to change the
paradigm too much but I needed to be able to add my custom cell data to the
file I decided to pass my data to the p{4,8}est_vtk_write_header()
function. I think that this makes the interface much messier and I don't
really like it, but before I went and refactored the code too much I
thought that I'd touch base with you all first.

Is custom cell data export something that you'd like to support in the
code? If it is I'd be happy to do whatever it takes to get the patch ready
to go. I have a branch called vtk_cell_data in my fork on github.com. If
you want you can peruse the changes at

https://github.com/advocateddrummer/p4est/compare/vtk_cell_data

Like I said, I think this code quite kludgy; for example: for now I have
copied and tweaked the p{4,8}est_vtk_write_point_{scalar,vector} functions
to write cell data. These functions re-open the vtu file and seek to the
end to append the point data; as I've added this logic into the
p{4,8}est_vtk_write_header() function I have to close the file before I
write the custom cell data and then re-open it after I'm done so that
p{4,8}est_vtk_write_header() can finish. It also does not match the
paradigm you have set up for custom point data. Furthermore, It adds a lot
of noise to the p{4,8}est_vtk_write_header() interface.

I have an idea of how I'd refactor this logic if it were up to me. I would
move all of the cell data export logic out of the
p{4,8}est_vtk_write_header() function and write the treeid/level/mpirank
cell data after p{4,8}est_vtk_write_header(), perhaps wrapping this into
its own function as this data is intrinsically part of the the p{4,8}est
data. Then I would handle the custom cell data much like the custom point
data is handled currently. I don't know if this would violate any
conventions you have though; I am certainly not the best person to be
making these decisions.

If you are interested in having this functionality I'd be thrilled to
create the patch. Just give me your thoughts and let me know how you'd like
me to implement it. If, on the other hand, you don't want this to be part
of the p4est codeset, that's also fine. I'll just maintain it in my own
branch.

Folks, thanks a ton for this great software. I am thrilled to be a part of
it, however small!

Cheers,

Ethan Alan


-- 
Ethan Alan

Re: [p4est] Custom cell data in the vtk exporter

From:
Carsten Burstedde
Date:
2014-10-07 @ 13:40
Ethan Alan,

> I have gone ahead and added logic that allows for custom cell data to be
> exported to the vtu files. It seems to work like a charm so far but it is
> pretty kludgy. The first thing I tried was to simply copy the way that
> custom point data is written; this does not work however as there can not
> be more than one <CellData Scalars> ... </CellData> sections in the vtu
> file (as far as I can tell anyway).

that's good to know.

> This created a bit of a dilemma for me as the cell data that is already
> exported (treeid, level, mpirank) is written in the
> p{4,8}est_vtk_write_header() function. As I didn't want to change the
> paradigm too much but I needed to be able to add my custom cell data to the
> file I decided to pass my data to the p{4,8}est_vtk_write_header()
> function. I think that this makes the interface much messier and I don't
> really like it, but before I went and refactored the code too much I
> thought that I'd touch base with you all first.

:)

> Is custom cell data export something that you'd like to support in the
> code? If it is I'd be happy to do whatever it takes to get the patch ready
> to go. I have a branch called vtk_cell_data in my fork on github.com. If
> you want you can peruse the changes at

Definitely, that feature is very welcome.

> https://github.com/advocateddrummer/p4est/compare/vtk_cell_data
> 
> Like I said, I think this code quite kludgy; for example: for now I have
> copied and tweaked the p{4,8}est_vtk_write_point_{scalar,vector} functions
> to write cell data. These functions re-open the vtu file and seek to the
> end to append the point data; as I've added this logic into the
> p{4,8}est_vtk_write_header() function I have to close the file before I
> write the custom cell data and then re-open it after I'm done so that
> p{4,8}est_vtk_write_header() can finish. It also does not match the
> paradigm you have set up for custom point data. Furthermore, It adds a lot
> of noise to the p{4,8}est_vtk_write_header() interface.

I agree.

> I have an idea of how I'd refactor this logic if it were up to me. I would
> move all of the cell data export logic out of the
> p{4,8}est_vtk_write_header() function and write the treeid/level/mpirank
> cell data after p{4,8}est_vtk_write_header(), perhaps wrapping this into
> its own function as this data is intrinsically part of the the p{4,8}est
> data. Then I would handle the custom cell data much like the custom point
> data is handled currently. I don't know if this would violate any
> conventions you have though; I am certainly not the best person to be
> making these decisions.

Separating the cell data sounds good.  What about a write_header
function that writes <Points> and <Cells>, then a separate function
write_cell_data (write_tree, level, rank, int num_scalars, int
num_vectors, ...) (null terminated list of the individual pairs
(fieldname, fielddata) as in _write_all, accessed with va_arg), and
similar for write_point_data but without the tree, level, rank?

> If you are interested in having this functionality I'd be thrilled to
> create the patch. Just give me your thoughts and let me know how you'd like
> me to implement it. If, on the other hand, you don't want this to be part
> of the p4est codeset, that's also fine. I'll just maintain it in my own
> branch.

Looking forward to a patch with the new function prototypes to talk it
over.  p4est_vtk_write_file and _write_all should stay unchanged (the
latter is for backwards compatibility -- could extend the docs to say
that it's for point data only).

> Folks, thanks a ton for this great software. I am thrilled to be a part of
> it, however small!

Thanks for the feedback!

Carsten

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2014-10-11 @ 17:08
Thank you Carsten.

I'm a little busy right now, but I'll get to this as soon as I can and let
you know.

Ethan Alan

Ethan Alan

On Tue, Oct 7, 2014 at 9:40 AM, Carsten Burstedde <burstedde@ins.uni-bonn.de
> wrote:

> Ethan Alan,
>
> > I have gone ahead and added logic that allows for custom cell data to be
> > exported to the vtu files. It seems to work like a charm so far but it is
> > pretty kludgy. The first thing I tried was to simply copy the way that
> > custom point data is written; this does not work however as there can not
> > be more than one <CellData Scalars> ... </CellData> sections in the vtu
> > file (as far as I can tell anyway).
>
> that's good to know.
>
> > This created a bit of a dilemma for me as the cell data that is already
> > exported (treeid, level, mpirank) is written in the
> > p{4,8}est_vtk_write_header() function. As I didn't want to change the
> > paradigm too much but I needed to be able to add my custom cell data to
> the
> > file I decided to pass my data to the p{4,8}est_vtk_write_header()
> > function. I think that this makes the interface much messier and I don't
> > really like it, but before I went and refactored the code too much I
> > thought that I'd touch base with you all first.
>
> :)
>
> > Is custom cell data export something that you'd like to support in the
> > code? If it is I'd be happy to do whatever it takes to get the patch
> ready
> > to go. I have a branch called vtk_cell_data in my fork on github.com. If
> > you want you can peruse the changes at
>
> Definitely, that feature is very welcome.
>
> > https://github.com/advocateddrummer/p4est/compare/vtk_cell_data
> >
> > Like I said, I think this code quite kludgy; for example: for now I have
> > copied and tweaked the p{4,8}est_vtk_write_point_{scalar,vector}
> functions
> > to write cell data. These functions re-open the vtu file and seek to the
> > end to append the point data; as I've added this logic into the
> > p{4,8}est_vtk_write_header() function I have to close the file before I
> > write the custom cell data and then re-open it after I'm done so that
> > p{4,8}est_vtk_write_header() can finish. It also does not match the
> > paradigm you have set up for custom point data. Furthermore, It adds a
> lot
> > of noise to the p{4,8}est_vtk_write_header() interface.
>
> I agree.
>
> > I have an idea of how I'd refactor this logic if it were up to me. I
> would
> > move all of the cell data export logic out of the
> > p{4,8}est_vtk_write_header() function and write the treeid/level/mpirank
> > cell data after p{4,8}est_vtk_write_header(), perhaps wrapping this into
> > its own function as this data is intrinsically part of the the p{4,8}est
> > data. Then I would handle the custom cell data much like the custom point
> > data is handled currently. I don't know if this would violate any
> > conventions you have though; I am certainly not the best person to be
> > making these decisions.
>
> Separating the cell data sounds good.  What about a write_header
> function that writes <Points> and <Cells>, then a separate function
> write_cell_data (write_tree, level, rank, int num_scalars, int
> num_vectors, ...) (null terminated list of the individual pairs
> (fieldname, fielddata) as in _write_all, accessed with va_arg), and
> similar for write_point_data but without the tree, level, rank?
>
> > If you are interested in having this functionality I'd be thrilled to
> > create the patch. Just give me your thoughts and let me know how you'd
> like
> > me to implement it. If, on the other hand, you don't want this to be part
> > of the p4est codeset, that's also fine. I'll just maintain it in my own
> > branch.
>
> Looking forward to a patch with the new function prototypes to talk it
> over.  p4est_vtk_write_file and _write_all should stay unchanged (the
> latter is for backwards compatibility -- could extend the docs to say
> that it's for point data only).
>
> > Folks, thanks a ton for this great software. I am thrilled to be a part
> of
> > it, however small!
>
> Thanks for the feedback!
>
> Carsten
>

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2014-10-24 @ 18:51
On Sat, Oct 11, 2014 at 1:08 PM, Ethan Hereth <advocateddrummer@gmail.com>
wrote:

> Thank you Carsten.
>
> I'm a little busy right now, but I'll get to this as soon as I can and let
> you know.
>
> Ethan Alan
>
> Ethan Alan
>
> On Tue, Oct 7, 2014 at 9:40 AM, Carsten Burstedde <
> burstedde@ins.uni-bonn.de> wrote:
>
>> Ethan Alan,
>>
>> > I have gone ahead and added logic that allows for custom cell data to be
>> > exported to the vtu files. It seems to work like a charm so far but it
>> is
>> > pretty kludgy. The first thing I tried was to simply copy the way that
>> > custom point data is written; this does not work however as there can
>> not
>> > be more than one <CellData Scalars> ... </CellData> sections in the vtu
>> > file (as far as I can tell anyway).
>>
>> that's good to know.
>>
>> > This created a bit of a dilemma for me as the cell data that is already
>> > exported (treeid, level, mpirank) is written in the
>> > p{4,8}est_vtk_write_header() function. As I didn't want to change the
>> > paradigm too much but I needed to be able to add my custom cell data to
>> the
>> > file I decided to pass my data to the p{4,8}est_vtk_write_header()
>> > function. I think that this makes the interface much messier and I don't
>> > really like it, but before I went and refactored the code too much I
>> > thought that I'd touch base with you all first.
>>
>> :)
>>
>> > Is custom cell data export something that you'd like to support in the
>> > code? If it is I'd be happy to do whatever it takes to get the patch
>> ready
>> > to go. I have a branch called vtk_cell_data in my fork on github.com.
>> If
>> > you want you can peruse the changes at
>>
>> Definitely, that feature is very welcome.
>>
>> > https://github.com/advocateddrummer/p4est/compare/vtk_cell_data
>> >
>> > Like I said, I think this code quite kludgy; for example: for now I have
>> > copied and tweaked the p{4,8}est_vtk_write_point_{scalar,vector}
>> functions
>> > to write cell data. These functions re-open the vtu file and seek to the
>> > end to append the point data; as I've added this logic into the
>> > p{4,8}est_vtk_write_header() function I have to close the file before I
>> > write the custom cell data and then re-open it after I'm done so that
>> > p{4,8}est_vtk_write_header() can finish. It also does not match the
>> > paradigm you have set up for custom point data. Furthermore, It adds a
>> lot
>> > of noise to the p{4,8}est_vtk_write_header() interface.
>>
>> I agree.
>>
>> > I have an idea of how I'd refactor this logic if it were up to me. I
>> would
>> > move all of the cell data export logic out of the
>> > p{4,8}est_vtk_write_header() function and write the treeid/level/mpirank
>> > cell data after p{4,8}est_vtk_write_header(), perhaps wrapping this into
>> > its own function as this data is intrinsically part of the the p{4,8}est
>> > data. Then I would handle the custom cell data much like the custom
>> point
>> > data is handled currently. I don't know if this would violate any
>> > conventions you have though; I am certainly not the best person to be
>> > making these decisions.
>>
>> Separating the cell data sounds good.  What about a write_header
>> function that writes <Points> and <Cells>, then a separate function
>> write_cell_data (write_tree, level, rank, int num_scalars, int
>> num_vectors, ...) (null terminated list of the individual pairs
>> (fieldname, fielddata) as in _write_all, accessed with va_arg), and
>> similar for write_point_data but without the tree, level, rank?
>>
>> > If you are interested in having this functionality I'd be thrilled to
>> > create the patch. Just give me your thoughts and let me know how you'd
>> like
>> > me to implement it. If, on the other hand, you don't want this to be
>> part
>> > of the p4est codeset, that's also fine. I'll just maintain it in my own
>> > branch.
>>
>> Looking forward to a patch with the new function prototypes to talk it
>> over.  p4est_vtk_write_file and _write_all should stay unchanged (the
>> latter is for backwards compatibility -- could extend the docs to say
>> that it's for point data only).
>>
>> > Folks, thanks a ton for this great software. I am thrilled to be a part
>> of
>> > it, however small!
>>
>> Thanks for the feedback!
>>
>> Carsten
>>
>
> Good day Carsten,

I have finally gotten around to implementing this and I've pushed the patch
to the vtk_cell_data branch of my fork of p4est. I'm quite interested in
you thoughts. As usual, things are never as simple as they first appear!

I should note that I've tested this pretty extensively in both ASCII and
binary mode, writing out point data only, cell data only, both point and
cell data, etc. and every thing worked for me as expected. The project
builds and passes all tests with no warnings (-Wall) in any of the
p4est_vtk_* functions. Furthermore I've run at least one test case through
valgrind with no memory related issues.

Please refer to commit log here

https://github.com/advocateddrummer/p4est/commit/2e51bc49d84f61e0b1d562e34a5a0ff04c65a494
for most of the details.

I am specifically interested in your thoughts concerning the
vp4est_vtk_write_point_data function; as you'll see,
p4est_vtk_write_point_data is just a wrapper to this function which does
all of the work. When it became apparent to me that I needed to pass
variable arguments from p4est_vtk_write_all to p4est_vtk_write_point_data
to keep the code clean and eliminate duplicated code I wasn't sure what to
do. I came across the following FAQ:
http://www.c-faq.com/varargs/handoff.html and used ideas expressed there to
accomplish my goal. That is also where I came up with the naming convention
I used (prefixing the function with a v.) It does not really fit with the
naming conventions used in p4est so I'll wait to hear what you think about
it. I at least hope that the general solution is what you had in mind.

Also notice that I've extended the documentation to reflect my changes. I
think I've done this fully/completely but you might want to look it over
and make sure it looks ok to you.

I have a few other questions/concerns:

1) The  p4est_geometry_t * geom argument gets passed around a lot but it is
only used in p4est_vtk_write_header. Is there a reason for this? Should it
stay or could it be eliminated in the other functions?

2) There is A LOT of file opening and closing and re-opening and re-closing
as the file name gets passed around instead of a file pointer. I don't know
if you are concerned about this but it seems sub-optimal to me.

3) Are you ok with introducing a p4est_vtk_write_all_ext function that
would support both custom point and cell data? I would be happy to do the
work involved for this. I assume that you'd want the declaration to live in
p{4,8}est_extended.h like the others? In the current implementation, this
would require a vp4est_vtk_write_cell_data function be created just like
the vp4est_vtk_write_point_data.

I think that is about it. I look forward to your feedback. Thanks again for
letting me help.

Ethan Alan

Re: [p4est] Custom cell data in the vtk exporter

From:
Carsten Burstedde
Date:
2014-11-14 @ 16:47
Hi Ethan,

> I have finally gotten around to implementing this and I've pushed the patch
> to the vtk_cell_data branch of my fork of p4est. I'm quite interested in
> you thoughts. As usual, things are never as simple as they first appear!
> 
> I should note that I've tested this pretty extensively in both ASCII and
> binary mode, writing out point data only, cell data only, both point and
> cell data, etc. and every thing worked for me as expected. The project
> builds and passes all tests with no warnings (-Wall) in any of the
> p4est_vtk_* functions. Furthermore I've run at least one test case through
> valgrind with no memory related issues.
> 
> Please refer to commit log here
> 
https://github.com/advocateddrummer/p4est/commit/2e51bc49d84f61e0b1d562e34a5a0ff04c65a494
> for most of the details.

great, I've made a vtk branch on github to follow up.

> I am specifically interested in your thoughts concerning the
> vp4est_vtk_write_point_data function; as you'll see,
> p4est_vtk_write_point_data is just a wrapper to this function which does
> all of the work. When it became apparent to me that I needed to pass
> variable arguments from p4est_vtk_write_all to p4est_vtk_write_point_data
> to keep the code clean and eliminate duplicated code I wasn't sure what to
> do. I came across the following FAQ:
> http://www.c-faq.com/varargs/handoff.html and used ideas expressed there to
> accomplish my goal. That is also where I came up with the naming convention
> I used (prefixing the function with a v.) It does not really fit with the
> naming conventions used in p4est so I'll wait to hear what you think about
> it. I at least hope that the general solution is what you had in mind.

I like the varargs solution.  Would you want to add an analogous
function for the cell data (and put the v as the last letter in the
function name)?  We should also require that there is an extra final
argument that must be some pointer we know, such as p4est or a magic
member of the buffer structure proposed below, after all the data pairs
for a bit of calling redundancy / segv protection.

> Also notice that I've extended the documentation to reflect my changes. I
> think I've done this fully/completely but you might want to look it over
> and make sure it looks ok to you.
> 
> I have a few other questions/concerns:
> 
> 1) The  p4est_geometry_t * geom argument gets passed around a lot but it is
> only used in p4est_vtk_write_header. Is there a reason for this? Should it
> stay or could it be eliminated in the other functions?

I'd be fine removing it from all functions where it is not used.

> 2) There is A LOT of file opening and closing and re-opening and re-closing
> as the file name gets passed around instead of a file pointer. I don't know
> if you are concerned about this but it seems sub-optimal to me.

That's certainly right.  Let's stabilize the current line of work and
then think about an alternative write_header_ext returning an allocated
buffer struct with all data that is required, passed to all subsequent
functions, and freed by write_footer_ext (similarly to
p4est_lnodes_share_owned_begin, end).  We may want to keep the current
API around for backwards compatibility, so handling both ways
non-redundantly on the inside would be nice.  The write_file convenience
functions would use the new API internally.

> 3) Are you ok with introducing a p4est_vtk_write_all_ext function that
> would support both custom point and cell data? I would be happy to do the
> work involved for this. I assume that you'd want the declaration to live in
> p{4,8}est_extended.h like the others? In the current implementation, this
> would require a vp4est_vtk_write_cell_data function be created just like
> the vp4est_vtk_write_point_data.

If someone needs it, sure.  Otherwise I'd wait.

> I think that is about it. I look forward to your feedback. Thanks again for
> letting me help.

There is one code safety issue that goes back a long time:  It is
neither documented nor checked that the data arrays have the correct
length.  We could turn the pairs into triples including the number of
doubles in each array (or using an sc_array rightaway -- maybe better).
Basically every array would come with a length that will be checked.
This would be enforced in the new API.  We'd of course also need
documentation that spells out how many doubles precisely we need for the
cell and point arrays.

Any thoughts on this last point?  Of course, if you're up to implement
this that would again be very valuable.

Thanks a lot for your work,

Carsten

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2014-12-09 @ 04:36
On Fri, Nov 14, 2014 at 11:47 AM, Carsten Burstedde <
burstedde@ins.uni-bonn.de> wrote:

Carsten,

I have been meaning to touch base with you about this for some time now but
I've been pretty swamped. I haven't forgotten about this and I look forward
getting back to working on it. Thanks for your patience.

>
> > I have finally gotten around to implementing this and I've pushed the
> patch
> > to the vtk_cell_data branch of my fork of p4est. I'm quite interested in
> > you thoughts. As usual, things are never as simple as they first appear!
> >
> > I should note that I've tested this pretty extensively in both ASCII and
> > binary mode, writing out point data only, cell data only, both point and
> > cell data, etc. and every thing worked for me as expected. The project
> > builds and passes all tests with no warnings (-Wall) in any of the
> > p4est_vtk_* functions. Furthermore I've run at least one test case
> through
> > valgrind with no memory related issues.
> >
> > Please refer to commit log here
> >
> 
https://github.com/advocateddrummer/p4est/commit/2e51bc49d84f61e0b1d562e34a5a0ff04c65a494
> > for most of the details.
>
> great, I've made a vtk branch on github to follow up.
>
> > I am specifically interested in your thoughts concerning the
> > vp4est_vtk_write_point_data function; as you'll see,
> > p4est_vtk_write_point_data is just a wrapper to this function which does
> > all of the work. When it became apparent to me that I needed to pass
> > variable arguments from p4est_vtk_write_all to p4est_vtk_write_point_data
> > to keep the code clean and eliminate duplicated code I wasn't sure what
> to
> > do. I came across the following FAQ:
> > http://www.c-faq.com/varargs/handoff.html and used ideas expressed
> there to
> > accomplish my goal. That is also where I came up with the naming
> convention
> > I used (prefixing the function with a v.) It does not really fit with the
> > naming conventions used in p4est so I'll wait to hear what you think
> about
> > it. I at least hope that the general solution is what you had in mind.
>
> I like the varargs solution.  Would you want to add an analogous
> function for the cell data (and put the v as the last letter in the
> function name)?  We should also require that there is an extra final
> argument that must be some pointer we know, such as p4est or a magic
> member of the buffer structure proposed below, after all the data pairs
> for a bit of calling redundancy / segv protection.
>
>
I'm glad you liked it... I'd be happy to add the cell data version and
change the naming convention too.


> > Also notice that I've extended the documentation to reflect my changes. I
> > think I've done this fully/completely but you might want to look it over
> > and make sure it looks ok to you.
> >
> > I have a few other questions/concerns:
> >
> > 1) The  p4est_geometry_t * geom argument gets passed around a lot but it
> is
> > only used in p4est_vtk_write_header. Is there a reason for this? Should
> it
> > stay or could it be eliminated in the other functions?
>
> I'd be fine removing it from all functions where it is not used.
>

I'll do that then first thing when I have time to get back to this.


>
> > 2) There is A LOT of file opening and closing and re-opening and
> re-closing
> > as the file name gets passed around instead of a file pointer. I don't
> know
> > if you are concerned about this but it seems sub-optimal to me.
>
> That's certainly right.  Let's stabilize the current line of work and
> then think about an alternative write_header_ext returning an allocated
> buffer struct with all data that is required, passed to all subsequent
> functions, and freed by write_footer_ext (similarly to
> p4est_lnodes_share_owned_begin, end).  We may want to keep the current
> API around for backwards compatibility, so handling both ways
> non-redundantly on the inside would be nice.  The write_file convenience
> functions would use the new API internally.
>
>
I'll take a peek at p4est_lnodes_share_owned_* to see what you are
referring to here; I think you are suggesting that the new write_header_ext
returns an allocated struct with, among other things, an opened file buffer
that all subsequent p4est_vtk_* functions would write to and
p?est_vtk_write_footer_ext would close? That seems like a good way to deal
with this issue. I wonder, what data would you like in that struct? And how
would it be passed around?

> 3) Are you ok with introducing a p4est_vtk_write_all_ext function that
> > would support both custom point and cell data? I would be happy to do the
> > work involved for this. I assume that you'd want the declaration to live
> in
> > p{4,8}est_extended.h like the others? In the current implementation, this
> > would require a vp4est_vtk_write_cell_data function be created just like
> > the vp4est_vtk_write_point_data.
>
> If someone needs it, sure.  Otherwise I'd wait.
>
> > I think that is about it. I look forward to your feedback. Thanks again
> for
> > letting me help.
>
> There is one code safety issue that goes back a long time:  It is
> neither documented nor checked that the data arrays have the correct
> length.  We could turn the pairs into triples including the number of
> doubles in each array (or using an sc_array rightaway -- maybe better).
> Basically every array would come with a length that will be checked.
> This would be enforced in the new API.  We'd of course also need
> documentation that spells out how many doubles precisely we need for the
> cell and point arrays.
>
> Any thoughts on this last point?  Of course, if you're up to implement
> this that would again be very valuable.
>

I have very little experience with implementing a proper API, so I would
default to your preferences here. I certainly think one can never be too
safe when it comes to stuff like this so I certainly think it would be a
great idea to check/validate the input. As far as how, I guess either way
you've mentioned would work. I am also not terribly familiar with the sc_*
utilities but I am all for using the sc_arrays if they already have bounds
checking built in and p?est already uses sc_* tools so much it seems
natural and p?est already depends upon it so it wouldn't be adding a
dependancy.

Once again, I am thrilled to do this, I'll just have to familiarize myself
with the sc_array struct and its usage. I'd also update the documentation
accordingly.

I hope to have time soon to get back to this.

Cheers,


> Thanks a lot for your work,
>
> Carsten
>

Re: [p4est] Custom cell data in the vtk exporter

From:
Carsten Burstedde
Date:
2014-12-11 @ 13:10
> I have been meaning to touch base with you about this for some time now but
> I've been pretty swamped. I haven't forgotten about this and I look forward
> getting back to working on it. Thanks for your patience.

Thanks, it's all good.

> > > I have finally gotten around to implementing this and I've pushed the
> > patch
> > > to the vtk_cell_data branch of my fork of p4est. I'm quite interested in
> > > you thoughts. As usual, things are never as simple as they first appear!
> > >
> > > I should note that I've tested this pretty extensively in both ASCII and
> > > binary mode, writing out point data only, cell data only, both point and
> > > cell data, etc. and every thing worked for me as expected. The project
> > > builds and passes all tests with no warnings (-Wall) in any of the
> > > p4est_vtk_* functions. Furthermore I've run at least one test case
> > through
> > > valgrind with no memory related issues.
> > >
> > > Please refer to commit log here
> > >
> > 
https://github.com/advocateddrummer/p4est/commit/2e51bc49d84f61e0b1d562e34a5a0ff04c65a494
> > > for most of the details.
> >
> > great, I've made a vtk branch on github to follow up.

I've bumped it with some API propositions.

> > > I am specifically interested in your thoughts concerning the
> > > vp4est_vtk_write_point_data function; as you'll see,
> > > p4est_vtk_write_point_data is just a wrapper to this function which does
> > > all of the work. When it became apparent to me that I needed to pass
> > > variable arguments from p4est_vtk_write_all to p4est_vtk_write_point_data
> > > to keep the code clean and eliminate duplicated code I wasn't sure what
> > to
> > > do. I came across the following FAQ:
> > > http://www.c-faq.com/varargs/handoff.html and used ideas expressed
> > there to
> > > accomplish my goal. That is also where I came up with the naming
> > convention
> > > I used (prefixing the function with a v.) It does not really fit with the
> > > naming conventions used in p4est so I'll wait to hear what you think
> > about
> > > it. I at least hope that the general solution is what you had in mind.
> >
> > I like the varargs solution.  Would you want to add an analogous
> > function for the cell data (and put the v as the last letter in the
> > function name)?  We should also require that there is an extra final
> > argument that must be some pointer we know, such as p4est or a magic
> > member of the buffer structure proposed below, after all the data pairs
> > for a bit of calling redundancy / segv protection.
> >
> >
> I'm glad you liked it... I'd be happy to add the cell data version and
> change the naming convention too.

Cool.  I've put a couple TODO comments into my vtk branch.

> > > Also notice that I've extended the documentation to reflect my changes. I
> > > think I've done this fully/completely but you might want to look it over
> > > and make sure it looks ok to you.
> > >
> > > I have a few other questions/concerns:
> > >
> > > 1) The  p4est_geometry_t * geom argument gets passed around a lot but it
> > is
> > > only used in p4est_vtk_write_header. Is there a reason for this? Should
> > it
> > > stay or could it be eliminated in the other functions?
> >
> > I'd be fine removing it from all functions where it is not used.
> >
> 
> I'll do that then first thing when I have time to get back to this.

This will be a non-issue when we move to the context buffer.  It doesn't
hurt to store a couple extra fields in the context data structure.

> > > 2) There is A LOT of file opening and closing and re-opening and
> > re-closing
> > > as the file name gets passed around instead of a file pointer. I don't
> > know
> > > if you are concerned about this but it seems sub-optimal to me.
> >
> > That's certainly right.  Let's stabilize the current line of work and
> > then think about an alternative write_header_ext returning an allocated
> > buffer struct with all data that is required, passed to all subsequent
> > functions, and freed by write_footer_ext (similarly to
> > p4est_lnodes_share_owned_begin, end).  We may want to keep the current
> > API around for backwards compatibility, so handling both ways
> > non-redundantly on the inside would be nice.  The write_file convenience
> > functions would use the new API internally.
> >
> >
> I'll take a peek at p4est_lnodes_share_owned_* to see what you are
> referring to here; I think you are suggesting that the new write_header_ext
> returns an allocated struct with, among other things, an opened file buffer
> that all subsequent p4est_vtk_* functions would write to and
> p?est_vtk_write_footer_ext would close? That seems like a good way to deal
> with this issue. I wonder, what data would you like in that struct? And how
> would it be passed around?

Yes, that's the idea.  Please see my current vtk branch and extend as
needed.

> > 3) Are you ok with introducing a p4est_vtk_write_all_ext function that
> > > would support both custom point and cell data? I would be happy to do the
> > > work involved for this. I assume that you'd want the declaration to live
> > in
> > > p{4,8}est_extended.h like the others? In the current implementation, this
> > > would require a vp4est_vtk_write_cell_data function be created just like
> > > the vp4est_vtk_write_point_data.
> >
> > If someone needs it, sure.  Otherwise I'd wait.
> >
> > > I think that is about it. I look forward to your feedback. Thanks again
> > for
> > > letting me help.
> >
> > There is one code safety issue that goes back a long time:  It is
> > neither documented nor checked that the data arrays have the correct
> > length.  We could turn the pairs into triples including the number of
> > doubles in each array (or using an sc_array rightaway -- maybe better).
> > Basically every array would come with a length that will be checked.
> > This would be enforced in the new API.  We'd of course also need
> > documentation that spells out how many doubles precisely we need for the
> > cell and point arrays.
> >
> > Any thoughts on this last point?  Of course, if you're up to implement
> > this that would again be very valuable.
> >
> 
> I have very little experience with implementing a proper API, so I would
> default to your preferences here. I certainly think one can never be too
> safe when it comes to stuff like this so I certainly think it would be a
> great idea to check/validate the input. As far as how, I guess either way
> you've mentioned would work. I am also not terribly familiar with the sc_*
> utilities but I am all for using the sc_arrays if they already have bounds
> checking built in and p?est already uses sc_* tools so much it seems
> natural and p?est already depends upon it so it wouldn't be adding a
> dependancy.

For the varargs version, we can require sc_array_t * for the double data
and require a final marker parameter at the end of the list that we
always check with an assertion.

I'm also thinking to add an alternate given-number-of-fields version
that people can feed with an array of fields, not varargs, so they can
use a for loop to create the arguments.  I'm not done defining it so
let's wait on this one (both cell and point data).

> Once again, I am thrilled to do this, I'll just have to familiarize myself
> with the sc_array struct and its usage. I'd also update the documentation
> accordingly.
> 
> I hope to have time soon to get back to this.

Awesome, looking forward to it.

Carsten

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2014-12-31 @ 22:52
Carsten,

Greetings! I hope you've had a great holiday season so far and that you
have a great new year.

I've finally made some time to get back to this vtk dev. I've pushed my
additions to my github branch for you to look over. I'm not totally done
and I've not thoroughly tested the changes yet but, so far, they seem to
work with one exception which I'll explain shortly. Please glance at what
I've done when you get a chance and let me know what you think.

On Thu, Dec 11, 2014 at 8:10 AM, Carsten Burstedde <
burstedde@ins.uni-bonn.de> wrote:
>
> > I have been meaning to touch base with you about this for some time now
> but
> > I've been pretty swamped. I haven't forgotten about this and I look
> forward
> > getting back to working on it. Thanks for your patience.
>
> Thanks, it's all good.


> > > > I have finally gotten around to implementing this and I've pushed the
> > > patch
> > > > to the vtk_cell_data branch of my fork of p4est. I'm quite
> interested in
> > > > you thoughts. As usual, things are never as simple as they first
> appear!
> > > >
> > > > I should note that I've tested this pretty extensively in both ASCII
> and
> > > > binary mode, writing out point data only, cell data only, both point
> and
> > > > cell data, etc. and every thing worked for me as expected. The
> project
> > > > builds and passes all tests with no warnings (-Wall) in any of the
> > > > p4est_vtk_* functions. Furthermore I've run at least one test case
> > > through
> > > > valgrind with no memory related issues.
> > > >
> > > > Please refer to commit log here
> > > >
> > >
> 
https://github.com/advocateddrummer/p4est/commit/2e51bc49d84f61e0b1d562e34a5a0ff04c65a494
> > > > for most of the details.
> > >
> > > great, I've made a vtk branch on github to follow up.
>
> I've bumped it with some API propositions.
>

Thanks for the propositions, they really helped clear up what you were
expecting/thinking.


> > > > I am specifically interested in your thoughts concerning the
> > > > vp4est_vtk_write_point_data function; as you'll see,
> > > > p4est_vtk_write_point_data is just a wrapper to this function which
> does
> > > > all of the work. When it became apparent to me that I needed to pass
> > > > variable arguments from p4est_vtk_write_all to
> p4est_vtk_write_point_data
> > > > to keep the code clean and eliminate duplicated code I wasn't sure
> what
> > > to
> > > > do. I came across the following FAQ:
> > > > http://www.c-faq.com/varargs/handoff.html and used ideas expressed
> > > there to
> > > > accomplish my goal. That is also where I came up with the naming
> > > convention
> > > > I used (prefixing the function with a v.) It does not really fit
> with the
> > > > naming conventions used in p4est so I'll wait to hear what you think
> > > about
> > > > it. I at least hope that the general solution is what you had in
> mind.
> > >
> > > I like the varargs solution.  Would you want to add an analogous
> > > function for the cell data (and put the v as the last letter in the
> > > function name)?  We should also require that there is an extra final
> > > argument that must be some pointer we know, such as p4est or a magic
> > > member of the buffer structure proposed below, after all the data pairs
> > > for a bit of calling redundancy / segv protection.
> > >
> > >
> > I'm glad you liked it... I'd be happy to add the cell data version and
> > change the naming convention too.
>
> Cool.  I've put a couple TODO comments into my vtk branch.
>
> > > > Also notice that I've extended the documentation to reflect my
> changes. I
> > > > think I've done this fully/completely but you might want to look it
> over
> > > > and make sure it looks ok to you.
> > > >
> > > > I have a few other questions/concerns:
> > > >
> > > > 1) The  p4est_geometry_t * geom argument gets passed around a lot
> but it
> > > is
> > > > only used in p4est_vtk_write_header. Is there a reason for this?
> Should
> > > it
> > > > stay or could it be eliminated in the other functions?
> > >
> > > I'd be fine removing it from all functions where it is not used.
> > >
> >
> > I'll do that then first thing when I have time to get back to this.
>
> This will be a non-issue when we move to the context buffer.  It doesn't
> hurt to store a couple extra fields in the context data structure.
>
> > > > 2) There is A LOT of file opening and closing and re-opening and
> > > re-closing
> > > > as the file name gets passed around instead of a file pointer. I
> don't
> > > know
> > > > if you are concerned about this but it seems sub-optimal to me.
> > >
> > > That's certainly right.  Let's stabilize the current line of work and
> > > then think about an alternative write_header_ext returning an allocated
> > > buffer struct with all data that is required, passed to all subsequent
> > > functions, and freed by write_footer_ext (similarly to
> > > p4est_lnodes_share_owned_begin, end).  We may want to keep the current
> > > API around for backwards compatibility, so handling both ways
> > > non-redundantly on the inside would be nice.  The write_file
> convenience
> > > functions would use the new API internally.
> > >
> > >
> > I'll take a peek at p4est_lnodes_share_owned_* to see what you are
> > referring to here; I think you are suggesting that the new
> write_header_ext
> > returns an allocated struct with, among other things, an opened file
> buffer
> > that all subsequent p4est_vtk_* functions would write to and
> > p?est_vtk_write_footer_ext would close? That seems like a good way to
> deal
> > with this issue. I wonder, what data would you like in that struct? And
> how
> > would it be passed around?
>
> Yes, that's the idea.  Please see my current vtk branch and extend as
> needed.
>

You'll see that I've implemented the p4est_vtk_context_t structure and
associated logic, added a p4est_vtk_context_destroy function, and done my
best to make sure everything is cleanly destroyed/free'd whenever there is
an error. This addition certainly cleans up all that nasty file
re-opening/re-closing, I like it. Please let me know if you have any
questions/concerns and I'll look into them as soon as I can.


>
> > > 3) Are you ok with introducing a p4est_vtk_write_all_ext function that
> > > > would support both custom point and cell data? I would be happy to
> do the
> > > > work involved for this. I assume that you'd want the declaration to
> live
> > > in
> > > > p{4,8}est_extended.h like the others? In the current implementation,
> this
> > > > would require a vp4est_vtk_write_cell_data function be created just
> like
> > > > the vp4est_vtk_write_point_data.
> > >
> > > If someone needs it, sure.  Otherwise I'd wait.
> > >
> > > > I think that is about it. I look forward to your feedback. Thanks
> again
> > > for
> > > > letting me help.
> > >
> > > There is one code safety issue that goes back a long time:  It is
> > > neither documented nor checked that the data arrays have the correct
> > > length.  We could turn the pairs into triples including the number of
> > > doubles in each array (or using an sc_array rightaway -- maybe better).
> > > Basically every array would come with a length that will be checked.
> > > This would be enforced in the new API.  We'd of course also need
> > > documentation that spells out how many doubles precisely we need for
> the
> > > cell and point arrays.
> > >
> > > Any thoughts on this last point?  Of course, if you're up to implement
> > > this that would again be very valuable.
> > >
> >
> > I have very little experience with implementing a proper API, so I would
> > default to your preferences here. I certainly think one can never be too
> > safe when it comes to stuff like this so I certainly think it would be a
> > great idea to check/validate the input. As far as how, I guess either way
> > you've mentioned would work. I am also not terribly familiar with the
> sc_*
> > utilities but I am all for using the sc_arrays if they already have
> bounds
> > checking built in and p?est already uses sc_* tools so much it seems
> > natural and p?est already depends upon it so it wouldn't be adding a
> > dependancy.
>
> For the varargs version, we can require sc_array_t * for the double data
> and require a final marker parameter at the end of the list that we
> always check with an assertion.
>

I've also started implementing these changes. I have something that seems
to mostly work now; one issue that I have currently is that for some reason
casting from double to float data in the case that P4EST_VTK_FLOAT_TYPE is
#define'd as float does not seem to work. I haven't dug into this too much
but as far as I can tell it should work as is. (Honestly, I'm not sure that
I have ever tested the vtk IO with float data though, so maybe it never
worked? I should try that out.) I understand that enforcing the user store
double data in the sc_array's and then casting said double data to float
data *should* work; am I missing something here?

Another thing, as I'm not as familiar with the correct/recommended usage of
the sc_* utilities, I'm not sure that I'm using/accessing the sc_arrays
correctly. Please look at what I've done and let me know if there is a
better way.

One other thing that I've not done yet either is add the 'final marker
parameter'. I'm not exactly sure what you want here. Do you mean that the
user should pass all the (data_name, data_array) pairs followed by the
p4est_vtk_context and internally I should check that the last argument is
the same as the context that was passed as the first argument?

I.e. p4est_vtk_write_cell_data (p4est_vtk_context_t * cont, write_tree,
write_level, write_rank, wrap_rank, num_cell_scalars, num_cell_vectors,
..., final) { /*gather variable arguments*/ P4EST_ASSERT(final == cont) ...
};?

Is this what you had in mind? This won't take me long to do, I just wanted
some clarification. Furthermore, is the data validation I've done so far
what you were expecting? The validation could also be done in
p4est_vtk_write_cell_{scalar,vector} although I'm not sure that makes as
much sense.

I have not yet eliminated the p4est_vtk_write_all function; this also won't
take long. I had a question though: do you want there to be no interface
for users who want to write extra, custom data? I.e. if a user wants to
write both custom point and cell data the code would look like:

cont = p4est_vtk_write_header(...)
cont = p4est_vtk_write_point_data(...)
cont = p4est_vtk_write_cell_data(...)
p4est_vtk_write_footer(cont)

I guess, now that I put it down, that really isn't that laborious! Unless
you say otherwise, I'll go ahead and remove that function shortly.

Something I recently realized: I'm not sure that the
p4est_vtk_write_{cell,point}_datav functions have much use without
p4est_vtk_write_all. The main reason I created them was to support calling
them from within p4est_vtk_write_all as I needed a way to call a function
that required variable arguments from within a function that itself
contained said variable argument list. I've already implemented them so
I've kept them around, but I wondered, did you have another plan for these?

Lastly, I've updated a lot of the documentation and added the documentation
you requested. I noticed that you made a few tweaks to the documentation
too and I wondered what you meant by the following addition to the
p4est_vtk_write_cell_data function docs:

 * There are options to have this function write
 * the tree id, quadrant level, or MPI rank without explicit input data.

Can you expound on that statement?


> I'm also thinking to add an alternate given-number-of-fields version
> that people can feed with an array of fields, not varargs, so they can
> use a for loop to create the arguments.  I'm not done defining it so
> let's wait on this one (both cell and point data).
>

Ok, sounds good, let me know what you know what you want here.


>
> > Once again, I am thrilled to do this, I'll just have to familiarize
> myself
> > with the sc_array struct and its usage. I'd also update the documentation
> > accordingly.
> >
> > I hope to have time soon to get back to this.
>
> Awesome, looking forward to it.
>
> I hope that all this makes sense, I tend to ramble I think! Thanks again!
I look forward to your input.

Happy New Year!

Ethan Alan


> Carsten
>

Re: [p4est] Custom cell data in the vtk exporter

From:
Carsten Burstedde
Date:
2015-01-16 @ 16:47
Ethan,

> Greetings! I hope you've had a great holiday season so far and that you
> have a great new year.

thanks, and to you too.

> I've finally made some time to get back to this vtk dev. I've pushed my
> additions to my github branch for you to look over. I'm not totally done
> and I've not thoroughly tested the changes yet but, so far, they seem to
> work with one exception which I'll explain shortly. Please glance at what
> I've done when you get a chance and let me know what you think.

This is very close, thanks!  I just have a couple comments (see below).

> You'll see that I've implemented the p4est_vtk_context_t structure and
> associated logic, added a p4est_vtk_context_destroy function, and done my
> best to make sure everything is cleanly destroyed/free'd whenever there is
> an error. This addition certainly cleans up all that nasty file
> re-opening/re-closing, I like it. Please let me know if you have any
> questions/concerns and I'll look into them as soon as I can.

We may want to hide the context better, that is, keep only the one-line
typedef in the .h and spell out the struct declaration in the .c without
the typedef.

As you mentioned, the datav/pointv functions are mostly internal, so you
could move them into the .c file, make them static, and remove them from
p4est_to_p8est.  (Personally I'd favor in the .h file to have all cell
functions first, then all point functions.)

There are a couple warnings related to const from compiling with -Wall.

The assertions on the user providing arrays of the correct type would
best be replaced with true checks and helpful error messages, since we
have no control about the calling code and we like to help out
beginners.

The sc_array_t always holds type double from the input, so the index is
always a double *.  The proper way of using it for floats would be:
float a = (float) (*sc_array_index (b, c));

There is one issue that we should probably address: It will in general
be impractical to require a different data format just depending on
scale.  I propose to always require data in the format for scale != 1.,
where the case scale == 1. just picks any of the possible corners
meeting at that node.  It's the user's responsibility to ensure
continuity.

I have a question to you too: does it work to write one cell or point
field at a time, with multiple calls to say write_cell_scalar, even
after write_cell_data has been called to write tree id etc.?

We'll come back to the fixed-number of fields prototypes later.  :)

> > For the varargs version, we can require sc_array_t * for the double data
> > and require a final marker parameter at the end of the list that we
> > always check with an assertion.
>
> One other thing that I've not done yet either is add the 'final marker
> parameter'. I'm not exactly sure what you want here. Do you mean that the
> user should pass all the (data_name, data_array) pairs followed by the
> p4est_vtk_context and internally I should check that the last argument is
> the same as the context that was passed as the first argument?
> 
> I.e. p4est_vtk_write_cell_data (p4est_vtk_context_t * cont, write_tree,
> write_level, write_rank, wrap_rank, num_cell_scalars, num_cell_vectors,
> ..., final) { /*gather variable arguments*/ P4EST_ASSERT(final == cont) ...
> };?
> 
> Is this what you had in mind? This won't take me long to do, I just wanted
> some clarification. Furthermore, is the data validation I've done so far
> what you were expecting? The validation could also be done in
> p4est_vtk_write_cell_{scalar,vector} although I'm not sure that makes as
> much sense.

Yes.  It would be best to use a true check, no assertion, with a very
descriptive error message on processor zero since first-time users will
get this wrong most of the time.

> cont = p4est_vtk_write_header(...)
> cont = p4est_vtk_write_point_data(...)
> cont = p4est_vtk_write_cell_data(...)
> p4est_vtk_write_footer(cont)
> 
> I guess, now that I put it down, that really isn't that laborious! Unless
> you say otherwise, I'll go ahead and remove that function shortly.

I think this looks just fine.

Your last question (can you expound?) was just about what you're doing
already (writing treeid, mpirank etc.).

Best,

Carsten

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2015-02-03 @ 16:26
Carsten,

I've pushed the new changes up to my github account; see below for details.
Please take a look when you have time and let me know what you think. Thank
you for being patient, I know this has been awhile coming...

On Fri, Jan 16, 2015 at 11:47 AM, Carsten Burstedde <
burstedde@ins.uni-bonn.de> wrote:

> Ethan,
>
> > Greetings! I hope you've had a great holiday season so far and that you
> > have a great new year.
>
> thanks, and to you too.
>
> > I've finally made some time to get back to this vtk dev. I've pushed my
> > additions to my github branch for you to look over. I'm not totally done
> > and I've not thoroughly tested the changes yet but, so far, they seem to
> > work with one exception which I'll explain shortly. Please glance at what
> > I've done when you get a chance and let me know what you think.
>
> This is very close, thanks!  I just have a couple comments (see below).
>
> > You'll see that I've implemented the p4est_vtk_context_t structure and
> > associated logic, added a p4est_vtk_context_destroy function, and done my
> > best to make sure everything is cleanly destroyed/free'd whenever there
> is
> > an error. This addition certainly cleans up all that nasty file
> > re-opening/re-closing, I like it. Please let me know if you have any
> > questions/concerns and I'll look into them as soon as I can.
>
> We may want to hide the context better, that is, keep only the one-line
> typedef in the .h and spell out the struct declaration in the .c without
> the typedef.
>
>
Sure thing; would you like to hide the p4est_vtk_context_destroy function
as well? Theoretically, no user should ever have to use this externally.


> As you mentioned, the datav/pointv functions are mostly internal, so you
> could move them into the .c file, make them static, and remove them from
> p4est_to_p8est.  (Personally I'd favor in the .h file to have all cell
> functions first, then all point functions.)
>

Done. Here is a code style question: do you like having the doxygen
documentation in the .c file? When I moved the datav/pointv functions into
the .c file, I moved the documentation with it; it seems a bit out of place
but I'll leave that up to you.


> There are a couple warnings related to const from compiling with -Wall.
>

I've fixed some of these by removing all the const modifiers on the
filename char *. I hate doing this as it really should be const but I don't
know of a better way to fix it as I'm setting the char * in the context to
the filename char * passed in. There is no way to do this without losing
the const modifier as far as I know; Is there a better way to deal with
this that I'm overlooking?

There are a couple others related to discarding the 'const' qualifier
passing argument one to sc_array_index. (Around lines 1172, 1241 in
p4est_vtk.c) This could be fixed by making sc_array_index expect a 'const
struct sc_array_t *' instead of a 'struct sc_array_t *' but I do not want
to modify code in sc without your consent. Or, I could make the 'values'
argument to p4est_vtk_write_{cell,point}_scalar simply be a sc_array_t *
without the const modifier. How do you want me to fix this?


>
> The assertions on the user providing arrays of the correct type would
> best be replaced with true checks and helpful error messages, since we
> have no control about the calling code and we like to help out
> beginners.
>

I've done this; please let me know if you'd like the messages to be more
descriptive.


>
> The sc_array_t always holds type double from the input, so the index is
> always a double *.  The proper way of using it for floats would be:
> float a = (float) (*sc_array_index (b, c));
>

Did you mean 'float a = *((float *) sc_array_index(b, c))'? I tried what
you specified above and it didn't work: sc_array_index returns a void
pointer and the compiler complained about de-referencing it.

>
> There is one issue that we should probably address: It will in general
> be impractical to require a different data format just depending on
> scale.  I propose to always require data in the format for scale != 1.,
> where the case scale == 1. just picks any of the possible corners
> meeting at that node.  It's the user's responsibility to ensure
> continuity.
>
>
I am not sure I follow you here Carsten, the only thing that varies with
the 'scale' parameter is the number of points/nodes written to the vtu
file. I personally think this is a valuable feature as significantly less
data gets dumped to the vtu file when scale = 1. But, I assume I'm simply
misunderstanding you here; can you be a little more specific please? Sorry
if I'm being dense :(


> I have a question to you too: does it work to write one cell or point
> field at a time, with multiple calls to say write_cell_scalar, even
> after write_cell_data has been called to write tree id etc.?
>

The short answer is no. I can think of a way to get around this but it
might not be the cleanest/slickest way to go. The problem is that a call to
write_cell_data wraps the cell data (scalars/vectors) with the

<CellData Scalars="...">
</CellData>

tags. I'm am not a vtu file expert, but as far as I know ALL of the cell
data has to exist between these two tags. I played around with multiple
<CellData Scalars> </CellData> tags with no luck.

I think it would be decently tractable to have another method to write
point/cell data where write_cell_header and write_cell_footer functions
could be used to wrap calls to p4est_vtk_write_cell_{cell,scalar} to
produce a valid vtu file (same thing for point data). In this case,
p4est_vtk_write_cell_header would be responsible for the rank, level, tree,
etc. output. I would be hard to enforce that all the data name strings get
included in the opening CellData tag like normal, but I don't think this is
a deal breaker.

This approach would allow for something like:

p4est_vtk_write_header()

p4est_vtk_write_cell_header()

for data in cellData
    p4est_vtk_write_cell_data()

p4est_write_cell_footer()

/* repeat for point data... */

p4est_vtk_write_footer()

I haven't thought this through extensively but I think something like this
could work. Thoughts? Concerns?


> We'll come back to the fixed-number of fields prototypes later.  :)
>
> > > For the varargs version, we can require sc_array_t * for the double
> data
> > > and require a final marker parameter at the end of the list that we
> > > always check with an assertion.
> >
> > One other thing that I've not done yet either is add the 'final marker
> > parameter'. I'm not exactly sure what you want here. Do you mean that the
> > user should pass all the (data_name, data_array) pairs followed by the
> > p4est_vtk_context and internally I should check that the last argument is
> > the same as the context that was passed as the first argument?
> >
> > I.e. p4est_vtk_write_cell_data (p4est_vtk_context_t * cont, write_tree,
> > write_level, write_rank, wrap_rank, num_cell_scalars, num_cell_vectors,
> > ..., final) { /*gather variable arguments*/ P4EST_ASSERT(final == cont)
> ...
> > };?
> >
> > Is this what you had in mind? This won't take me long to do, I just
> wanted
> > some clarification. Furthermore, is the data validation I've done so far
> > what you were expecting? The validation could also be done in
> > p4est_vtk_write_cell_{scalar,vector} although I'm not sure that makes as
> > much sense.
>
> Yes.  It would be best to use a true check, no assertion, with a very
> descriptive error message on processor zero since first-time users will
> get this wrong most of the time.
>

Please take a look at the message I've written and let me know what you
think. I've used SC_CHECK_ABORT to perform the check/abort, is that what
you had in mind?

>
> > cont = p4est_vtk_write_header(...)
> > cont = p4est_vtk_write_point_data(...)
> > cont = p4est_vtk_write_cell_data(...)
> > p4est_vtk_write_footer(cont)
> >
> > I guess, now that I put it down, that really isn't that laborious! Unless
> > you say otherwise, I'll go ahead and remove that function shortly.
>
> I think this looks just fine.
>
> Your last question (can you expound?) was just about what you're doing
> already (writing treeid, mpirank etc.).
>
> Best,
>
> Carsten
>

Cheers,

Ethan Alan Hereth

Re: [p4est] Custom cell data in the vtk exporter

From:
Carsten Burstedde
Date:
2015-03-05 @ 17:39
Ethan,

> I've pushed the new changes up to my github account; see below for details.
> Please take a look when you have time and let me know what you think. Thank
> you for being patient, I know this has been awhile coming...

thanks a lot, and I've been the slow one.  I've grabbed your changes and
mostly tweaked the docs a bit; see my vtk branch.

> > There are a couple warnings related to const from compiling with -Wall.

I rolled back the const changes, it seems the warnings were not easy to avoid.

> > The sc_array_t always holds type double from the input, so the index is
> > always a double *.  The proper way of using it for floats would be:
> > float a = (float) (*sc_array_index (b, c));
> 
> Did you mean 'float a = *((float *) sc_array_index(b, c))'? I tried what
> you specified above and it didn't work: sc_array_index returns a void
> pointer and the compiler complained about de-referencing it.

It needs to be (vtktype) *(double *), not *(vtktype *), this may still
need fixing.

> > There is one issue that we should probably address: It will in general
> > be impractical to require a different data format just depending on
> > scale.  I propose to always require data in the format for scale != 1.,
> > where the case scale == 1. just picks any of the possible corners
> > meeting at that node.  It's the user's responsibility to ensure
> > continuity.
> >
> I am not sure I follow you here Carsten, the only thing that varies with
> the 'scale' parameter is the number of points/nodes written to the vtu
> file. I personally think this is a valuable feature as significantly less
> data gets dumped to the vtu file when scale = 1. But, I assume I'm simply
> misunderstanding you here; can you be a little more specific please? Sorry
> if I'm being dense :(

This just applies to point values: I agree that it's good to save on the
points written when scale == 1, and I would not want to change that.
The problem is that currently the number of expected data values in the
arrays depends on scale (CHECK_ABORT checks against cont->num_nodes).
What's worse is that this depends on p4est_nodes, which I'd like to
deprecate from public use.  I'm thinking to enforce the input arrays to
have CHILDREN * cells many data points regardless of scale, and do the
subindexing on the inside of p4est_vtk.c when scale == 1, assuming we
have been called with a continuous point data field.

(You're inheriting some history here, thanks for staying with us.)

> This approach would allow for something like:
> 
> p4est_vtk_write_header()
> 
> p4est_vtk_write_cell_header()
> 
> for data in cellData
>     p4est_vtk_write_cell_data()
> 
> p4est_write_cell_footer()
> 
> /* repeat for point data... */
> 
> p4est_vtk_write_footer()

That would probably be taking it too far.  It's fine as is.

Best,

Carsten

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2015-04-01 @ 01:26
Carsten,

On Thu, Mar 5, 2015 at 12:39 PM, Carsten Burstedde <
burstedde@ins.uni-bonn.de> wrote:

> Ethan,
>
> > I've pushed the new changes up to my github account; see below for
> details.
> > Please take a look when you have time and let me know what you think.
> Thank
> > you for being patient, I know this has been awhile coming...
>
> thanks a lot, and I've been the slow one.  I've grabbed your changes and
> mostly tweaked the docs a bit; see my vtk branch.
>
> > > There are a couple warnings related to const from compiling with -Wall.
>
> I rolled back the const changes, it seems the warnings were not easy to
> avoid.
>
> > > The sc_array_t always holds type double from the input, so the index is
> > > always a double *.  The proper way of using it for floats would be:
> > > float a = (float) (*sc_array_index (b, c));
> >
> > Did you mean 'float a = *((float *) sc_array_index(b, c))'? I tried what
> > you specified above and it didn't work: sc_array_index returns a void
> > pointer and the compiler complained about de-referencing it.
>
> It needs to be (vtktype) *(double *), not *(vtktype *), this may still
> need fixing.
>

I've fixed this in my branch. Although I'm not sure it really makes any
difference, but it should be correct now.

>
> > > There is one issue that we should probably address: It will in general
> > > be impractical to require a different data format just depending on
> > > scale.  I propose to always require data in the format for scale != 1.,
> > > where the case scale == 1. just picks any of the possible corners
> > > meeting at that node.  It's the user's responsibility to ensure
> > > continuity.
> > >
> > I am not sure I follow you here Carsten, the only thing that varies with
> > the 'scale' parameter is the number of points/nodes written to the vtu
> > file. I personally think this is a valuable feature as significantly less
> > data gets dumped to the vtu file when scale = 1. But, I assume I'm simply
> > misunderstanding you here; can you be a little more specific please?
> Sorry
> > if I'm being dense :(
>
> This just applies to point values: I agree that it's good to save on the
> points written when scale == 1, and I would not want to change that.
> The problem is that currently the number of expected data values in the
> arrays depends on scale (CHECK_ABORT checks against cont->num_nodes).
> What's worse is that this depends on p4est_nodes, which I'd like to
> deprecate from public use.  I'm thinking to enforce the input arrays to
> have CHILDREN * cells many data points regardless of scale, and do the
> subindexing on the inside of p4est_vtk.c when scale == 1, assuming we
> have been called with a continuous point data field.
>
> (You're inheriting some history here, thanks for staying with us.)
>

So, I'll just wait till you figure out how you want this to look then. As
far as I can tell what we have now works which makes me happy!

>
> > This approach would allow for something like:
> >
> > p4est_vtk_write_header()
> >
> > p4est_vtk_write_cell_header()
> >
> > for data in cellData
> >     p4est_vtk_write_cell_data()
> >
> > p4est_write_cell_footer()
> >
> > /* repeat for point data... */
> >
> > p4est_vtk_write_footer()
>
> That would probably be taking it too far.  It's fine as is.
>
> Best,
>
> Carsten


I'll sit tight and wait to hear from you.

Cheers!

Ethan Alan

>

Re: [p4est] Custom cell data in the vtk exporter

From:
Carsten Burstedde
Date:
2014-11-14 @ 17:53
> > 2) There is A LOT of file opening and closing and re-opening and re-closing
> > as the file name gets passed around instead of a file pointer. I don't know
> > if you are concerned about this but it seems sub-optimal to me.
> 
> That's certainly right.  Let's stabilize the current line of work and
> then think about an alternative write_header_ext returning an allocated
> buffer struct with all data that is required, passed to all subsequent
> functions, and freed by write_footer_ext (similarly to
> p4est_lnodes_share_owned_begin, end).  We may want to keep the current
> API around for backwards compatibility, so handling both ways
> non-redundantly on the inside would be nice.  The write_file convenience
> functions would use the new API internally.

I checked deal.ii and it only uses the p4est_vtk_write_file function
that we should keep as is.  My vote would be to remove
p4est_vtk_write_all and change the rest of the API to use a transient
buffer type without calling it _ext.

Opinions?

Carsten

Re: [p4est] Custom cell data in the vtk exporter

From:
Ethan Hereth
Date:
2014-12-09 @ 04:48
On Fri, Nov 14, 2014 at 12:53 PM, Carsten Burstedde <
burstedde@ins.uni-bonn.de> wrote:

> > > 2) There is A LOT of file opening and closing and re-opening and
> re-closing
> > > as the file name gets passed around instead of a file pointer. I don't
> know
> > > if you are concerned about this but it seems sub-optimal to me.
> >
> > That's certainly right.  Let's stabilize the current line of work and
> > then think about an alternative write_header_ext returning an allocated
> > buffer struct with all data that is required, passed to all subsequent
> > functions, and freed by write_footer_ext (similarly to
> > p4est_lnodes_share_owned_begin, end).  We may want to keep the current
> > API around for backwards compatibility, so handling both ways
> > non-redundantly on the inside would be nice.  The write_file convenience
> > functions would use the new API internally.
>
> I checked deal.ii and it only uses the p4est_vtk_write_file function
> that we should keep as is.  My vote would be to remove
> p4est_vtk_write_all and change the rest of the API to use a transient
> buffer type without calling it _ext.
>
> Opinions?
>
>
I'm all for that, it seems simple enough. I'm just not sure what this
transient buffer type should look like. Can you expound please?

Cheers,

Ethan Alan