search

ForestClaw / forestclaw

Quadtree/octree adaptive PDE solver based based on p4est.
http://www.forestclaw.org
BSD 2-Clause "Simplified" License
57 stars 21 forks source link

Add domain constructor for given p4est #289

Closed tim-griesbach closed 6 months ago

tim-griesbach commented 8 months ago

This PR adds a new domain constructor that constructs a domain for a given p4est. This function will be required for the ForestClaw restart functionality because we must construct a domain given a p4est that was read from a file.

cburstedde commented 8 months ago

Do we require any conditions on the data_size and the user_pointer of the p4est that we pass to the new function? Shall we document the ownership more explicitly?

tim-griesbach commented 8 months ago

Do we require any conditions on the data_size and the user_pointer of the p4est that we pass to the new function?

The user_pointer of the p4est must be NULL. I added this to the documentation and to be more explicit I also added an assertion to the function fclaw2d_domain_new_p4est although p4est_wrap_new_p4est already asserts on p4est->user_pointer == NULL. The data_size of the p4est is not touched by p4est_wrap_new_p4est and therefore there should be no conditions on the data size.

Shall we document the ownership more explicitly?

After the latest push we document the user_pointer condition explicitly and on the ownership in general we still have the line:

This function takes ownership of the passed p4est and its connectivity.

Do you think that all in all this is clear enough?

cburstedde commented 8 months ago

Do we require any conditions on the data_size and the user_pointer of the p4est that we pass to the new function?

The user_pointer of the p4est must be NULL. I added this to the documentation and to be more explicit I also added an assertion to the function fclaw2d_domain_new_p4est although p4est_wrap_new_p4est already asserts on p4est->user_pointer == NULL. The data_size of the p4est is not touched by p4est_wrap_new_p4est and therefore there should be no conditions on the data size.

Cool, this is helpful I think. Would it make sense to call p4est_reset_data with size 0 on the passed forest? It seems that in wrap we only do this in p4est_wrap_new_copy, while also with p4est_wrap_new_p4est we access p.user_int etc, which will not play nicely with nonzero data_size. There are no assertions on this currently. This may be an oversight in the design of the wrap, or at least warrants a study of that code.

Shall we document the ownership more explicitly?

After the latest push we document the user_pointer condition explicitly and on the ownership in general we still have the line:

This function takes ownership of the passed pest and its connectivity.

Do you think that all in all this is clear enough?

Ah, it's right there! Depending on what we do with the p4est passed, we may be a bit more wordy on the p4est parameter description itself.

tim-griesbach commented 8 months ago

Do we require any conditions on the data_size and the user_pointer of the p4est that we pass to the new function? The user_pointer of the p4est must be NULL. I added this to the documentation and to be more explicit I also added an assertion to the function fclaw2d_domain_new_p4est although p4est_wrap_new_p4est already asserts on p4est->user_pointer == NULL. The data_size of the p4est is not touched by p4est_wrap_new_p4est and therefore there should be no conditions on the data size.

Cool, this is helpful I think. Would it make sense to call p4est_reset_data with size 0 on the passed forest? It seems that in wrap we only do this in p4est_wrap_new_copy, while also with p4est_wrap_new_p4est we access p.user_int etc, which will not play nicely with nonzero data_size. There are no assertions on this currently. This may be an oversight in the design of the wrap, or at least warrants a study of that code.

In p4est_wrap_new_copy we create a deep copy of the given p4est and I think that this may be an additional justification for resetting the quadrant data since we may want to prevent having two p4est both pointing to the same quadrant data. However, in p4est_wrap_new_p4est we do not copy the given p4est but take its ownership. In that sense I think it may be sensible to not reset the quadrant data of the p4est because otherwise the quadrant data will be lost.

I agree that an access of p.user_int in p4est_wrap_new_p4est would cause problems with non-zero quadrant data but I could not find the access of p.user_int or in general of the union p in the function p4est_wrap_new_p4est.

Shall we document the ownership more explicitly? After the latest push we document the user_pointer condition explicitly and on the ownership in general we still have the line: This function takes ownership of the passed pest and its connectivity. Do you think that all in all this is clear enough?

Ah, it's right there! Depending on what we do with the p4est passed, we may be a bit more wordy on the p4est parameter description itself.

We could elaborate to what the user_pointer of the p4est is set, i.e. the created p4est_wrap.

cburstedde commented 8 months ago

In p4est_wrap_new_copy we create a deep copy of the given p4est and I think that this may be an additional justification for resetting the quadrant data since we may want to prevent having two p4est both pointing to the same quadrant data. However, in p4est_wrap_new_p4est we do not copy the given p4est but take its ownership. In that sense I think it may be sensible to not reset the quadrant data of the p4est because otherwise the quadrant data will be lost.

I agree that an access of p.user_int in p4est_wrap_new_p4est would cause problems with non-zero quadrant data but I could not find the access of p.user_int or in general of the union p in the function p4est_wrap_new_p4est.

I'd be suspecting that subsequent modiications of the wrap by refine and coarsen assign to p, which loses any pointer to the user data stored previously. It gets deallocated ok eventually by the mempool, but it may stop being accessible during the lifetime of the wrap already.

tim-griesbach commented 8 months ago

I'd be suspecting that subsequent modiications of the wrap by refine and coarsen assign to p, which loses any pointer to the user data stored previously. It gets deallocated ok eventually by the mempool, but it may stop being accessible during the lifetime of the wrap already.

Ah, I see. The callback functions refine_callback, replace_on_refine, coarsen_callback, replace_on_coarsen and replace_on_balance use p.user_int. Therefore, I would propose to reset the data size to 0 in the function p4est_wrap_new_p4est and assert in the function fclaw2d_domain_new_p4est p4est->data_size == 0 since this seems to be more consistent to the handling of the data size in p4est_wrap_new_copy.

cburstedde commented 8 months ago

In p4est_wrap_new_copy we create a deep copy of the given p4est and I think that this may be an additional justification for resetting the quadrant data since we may want to prevent having two p4est both pointing to the same quadrant data. However, in p4est_wrap_new_p4est we do not copy the given p4est but take its ownership. In that sense I think it may be sensible to not reset the quadrant data of the p4est because otherwise the quadrant data will be lost. I agree that an access of p.user_int in p4est_wrap_new_p4est would cause problems with non-zero quadrant data but I could not find the access of p.user_int or in general of the union p in the function p4est_wrap_new_p4est. I'd be suspecting that subsequent modiications of the wrap by refine and coarsen assign to p, which loses any pointer to the user data stored previously. It gets deallocated ok eventually by the mempool, but it may stop being accessible during the lifetime of the wrap already.

Ah, I see. The callback functions refine_callback, replace_on_refine, coarsen_callback, replace_on_coarsen and replace_on_balance use p.user_int. Therefore, I would propose to reset the data size to 0 in the function p4est_wrap_new_p4est and assert in the function fclaw2d_domain_new_p4est p4est->data_size == 0 since this seems to be more consistent to the handling of the data size in p4est_wrap_new_copy.

Ok! sounds good, and re-checking the doxygen content would be splendid.

tim-griesbach commented 8 months ago

Ok! sounds good, and re-checking the doxygen content would be splendid.

Alright, I created the p4est PR https://github.com/cburstedde/p4est/pull/252 to adjust the handing of the data size of the given p4est. I also updated the doxygen documentation in both p4est and ForestClaw. Moreover, I added an assertion for the data size in ForestClaw.

This ForestClaw PR should be not merged until the p4est PR https://github.com/cburstedde/p4est/pull/252 is merged and the corresponding p4est submodule update is committed in ForestClaw since otherwise the function fclaw2d_domain_new_p4est will not behave as documented.

tim-griesbach commented 8 months ago

This ForestClaw PR should be not merged until the p4est PR cburstedde/p4est#252 is merged and the corresponding p4est submodule update is committed in ForestClaw since otherwise the function fclaw2d_domain_new_p4est will not behave as documented.

Since the PR https://github.com/cburstedde/p4est/pull/252 is now merged into p4est develop and I updated the p4est submodule of ForestClaw, from my side this PR is now ready to be merged.

tim-griesbach commented 8 months ago

This ForestClaw PR should be not merged until the p4est PR cburstedde/p4est#252 is merged and the corresponding p4est submodule update is committed in ForestClaw since otherwise the function fclaw2d_domain_new_p4est will not behave as documented.

Since the PR cburstedde/p4est#252 is now merged into p4est develop and I updated the p4est submodule of ForestClaw, from my side this PR is now ready to be merged.

This PR is not yet ready to merge since first I want to recheck if there are still some submodule related problems with linking jansson that are unrelated to this PR but that we expericenced before while using the current p4est develop branch in ForestClaw.

tim-griesbach commented 8 months ago

This ForestClaw PR should be not merged until the p4est PR cburstedde/p4est#252 is merged and the corresponding p4est submodule update is committed in ForestClaw since otherwise the function fclaw2d_domain_new_p4est will not behave as documented.

Since the PR cburstedde/p4est#252 is now merged into p4est develop and I updated the p4est submodule of ForestClaw, from my side this PR is now ready to be merged.

This PR is not yet ready to merge since first I want to recheck if there are still some submodule related problems with linking jansson that are unrelated to this PR but that we expericenced before while using the current p4est develop branch in ForestClaw.

This PR can not be merged until the jansson related linking problems on some systems (cf. in particular https://github.com/cburstedde/libsc/pull/140 and https://github.com/cburstedde/libsc/pull/141) are resolved.

cburstedde commented 6 months ago

We have updated the p4est and libsc develop branches. These have all the latest CMake code in them. @scottaiton please let us know if anything is awry with these versions. The autoconf build should be working fine as usual.

scottaiton commented 6 months ago

The CMake builds work on my systems. I'll let you know if anyone runs into issues.

tim-griesbach commented 6 months ago

The CMake builds work on my systems. I'll let you know if anyone runs into issues.

I think the CMake build problems were architecture dependent and it may be required to update p4est and libsc to current develop since this PR only contained an update of the p4est and libsc to an older develop version.