Modify

Opened 3 years ago

Last modified 18 months ago

#1819 reopened enhancement

CarpetMask: add option to exclude boxes

Reported by: bmundim Owned by: Erik Schnetter
Priority: unset Milestone:
Component: Carpet Version: development version
Keywords: CarpetMask CarpetReduce Cc:

Description (last modified by Roland Haas)

I would like to be able to exclude boxes in addition to spherical surfaces when setting the CarpetMask::weight (which is used by CarpetReduce to exclude regions when performing
reductions). The following pull request implements this functionality:

https://bitbucket.org/eschnett/carpet/pull-requests/5/bcm-carpetmask/diff

I have some parameter files I used to test my implementation. I can turn them into testsuites after the revision of this ticket.

Attachments (3)

test_mask_exclude_boxes_exterior.par (2.9 KB) - added by bmundim 3 years ago.
test_mask_exclude_boxes.par (2.8 KB) - added by bmundim 3 years ago.
test_mask_exclude_regions.par (2.6 KB) - added by bmundim 3 years ago.

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by bmundim

Changed 3 years ago by bmundim

Attachment: test_mask_exclude_boxes.par added

Changed 3 years ago by bmundim

comment:1 Changed 3 years ago by Erik Schnetter

The code looks fine; please add the test cases.

comment:2 Changed 3 years ago by bmundim

Ok, I will add these three files as test cases and will work on another one with AMR.

comment:3 Changed 3 years ago by bmundim

Status: newreview

comment:4 Changed 20 months ago by Roland Haas

Bruno, Erik: since there are now test cases, should this go in (after the release I guess)?

comment:5 Changed 20 months ago by Erik Schnetter

Status: reviewreviewed_ok

Yes, this is now fine.

comment:6 Changed 20 months ago by bmundim

Note that I haven't run these tests in a while. Before committing, it would be a good idea rerunning these tests and/or taking a look at the data again, ie visualizing it. I am sorry I won't have time to do so before the release. Please feel free to give it a try if you have time. Thanks.

comment:7 Changed 18 months ago by Roland Haas

Status: reviewed_okreopened

Bruno: unfortunately the tests fail on my laptop. Unless they are fixed the patch cannot be applied. I tried this with your branch from you bitbucket repo as well as with a branch that I rebased onto master (rhaas/carpetmask in the main repo).

I get differences like this:

diff -rw --from repos/carpet/CarpetMask/test repos/carpet/CarpetMask/test/test_mask_exclude_boxes/weight.x.asc TEST/sim/CarpetMask/test_mask_exclude_boxes/weight.x.asc
37c37
< 0     0       0 0 0   34 22 22        0       0.6 0 0 0
---
> 0     0       0 0 0   34 22 22        0       0.6 0 0 1
84c84
< 1     0       0 0 0   34 22 22        1       0.6 0 0 0
---
> 1     0       0 0 0   34 22 22        1       0.6 0 0 1
131c131
< 2     0       0 0 0   34 22 22        2       0.6 0 0 0
---
> 2     0       0 0 0   34 22 22        2       0.6 0 0 1
diff -rw --from repos/carpet/CarpetMask/test repos/carpet/CarpetMask/test/test_mask_exclude_boxes_exterior/weight.x.asc TEST/sim/CarpetMask/test_mask_exclude_boxes_exterior/weight.x.asc
37c37
< 0     0       0 0 0   34 22 22        0       0.6 0 0 0
---
> 0     0       0 0 0   34 22 22        0       0.6 0 0 1
84c84
< 1     0       0 0 0   34 22 22        1       0.6 0 0 0
---
> 1     0       0 0 0   34 22 22        1       0.6 0 0 1
131c131
< 2     0       0 0 0   34 22 22        2       0.6 0 0 0
---
> 2     0       0 0 0   34 22 22        2       0.6 0 0 1

comment:8 Changed 18 months ago by Roland Haas

Description: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain Erik Schnetter.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Erik Schnetter to the specified user.
The owner will be changed from Erik Schnetter to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.