#1589: suspicious code for operator+ in bboxset2 --------------------+------------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: new Priority: major | Milestone: Component: Carpet | Version: development version Keywords: Carpet | --------------------+------------------------------------------------------- Operator+ in bboxset2 seems to actually implement operator^ but I don't quite understand what they are all doing so, please review.
When running with CARPET_DEBUG and CARPET_USE_BBOXSET2 the code dies with an assert due to a non-empty box intersection from inside CARPET_DEBUG code which seems to be due to the (invalid) assumption that multiple box.exterior would not overlap.
Finally the last patch fixes an possible obscure issue where CarpetIOHDF5 will ignore the checkpoint=no tag of a grid variable if the checkpoint file does indeed contain such a variable (since eg it was written with a code version that still had checkpoint=yes).
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: Carpet ---------------------+------------------------------------------------------ Changes (by anonymous):
* status: new => review
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: review Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: Carpet ---------------------+------------------------------------------------------
Comment (by eschnett):
Patch 0001 seems fine. I still have to digest the others.
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reviewed_ok Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: Carpet ---------------------+------------------------------------------------------ Changes (by eschnett):
* status: review => reviewed_ok
Comment:
Where is the {{{+}}} operator that combines different box.exteriors? Is this the one changed in patch 0003?
Operator {{{^}}} calculates the symmetric set difference. Due to the internal representation used by bboxset2, which is based on symmetric set differences, this operator is especially efficient. I believe the current code is correct, and the patch only makes things less efficient. Please do not apply it.
Patch 0003 looks correct. I do not know why the fine grid boundaries should be disjoint. Please apply it.
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: major | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: Carpet ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Applied as has dcd0afee1d6ce0572a789be017ccdd5b0d1824d3 and 60183e2b8852ad6f6261ac6cdb0d6ffc9cd4dcb4 of Carpet.
I have left out 0002 as requested. I added instead a comment to operator+ explaining that yes indeed this is correct and the symmetric difference of disjoint sets is the union of the sets as commit 0d17588fccdb2e4882d3e0edb4ce0e29544c20d8.
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: major | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: Carpet ---------------------+------------------------------------------------------
Comment (by rhaas):
Replying to [comment:3 eschnett]:
Where is the {{{+}}} operator that combines different box.exteriors? Is
this the one changed in patch 0003? yes, it is in patch 0003.
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: major | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: Carpet ---------------------+------------------------------------------------------
Comment (by eschnett):
A correction, for the record -- I misspoke slightly.
{{{operator+}}} is not the symmetric set difference, this is {{{operator^}}}. {{{operator+}}} is the set union (similar to {{{operator|}}}), but the caller guarantees that the sets are disjoint, which allows certain optimizations. In this case, {{{operator|}}} is the same as {{{operator^}}}, so we can use the (here much cheaper) {{{operator^}}} to implement {{{operator+}}}.
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: major | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: Carpet ---------------------+------------------------------------------------------ Changes (by eschnett):
* status: closed => reopened * resolution: fixed =>
Comment:
Roland, I do not see your commit on the master branch.
#1589: suspicious code for operator+ in bboxset2 ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: major | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: Carpet ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: reopened => closed * resolution: => fixed
Comment:
Sorry, I had forgotten to git push after running the tests. Done now.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit