Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix fits mask merge behaviour #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix fits mask merge behaviour #24

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions breizorro/breizorro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,13 @@ def resolve_island(isl_spec, mask_image, wcs, ignore_missing=False):
raise ValueError(f"coordinates {c} do not select a valid island")
return value

def add_regions(mask_image, regs, wcs):
def add_regions(mask_image, regs, wcs, logicaland=False):
for reg in regs:
if hasattr(reg, 'to_pixel'):
reg = reg.to_pixel(wcs)
mask_image += reg.to_mask().to_image(mask_image.shape)
mask_image = np.logical_and(mask_image, reg.to_mask().to_image(mask_image.shape)) \
if logicaland else np.logical_or(mask_image, reg.to_mask().to_image(mask_image.shape))


def remove_regions(mask_image, regs, wcs):
for reg in regs:
Expand Down Expand Up @@ -145,6 +147,8 @@ def main():
help='Merge in one or more masks or region files')
parser.add_argument('--subtract', dest='subtract', metavar="MASK(s)|REG(s)", nargs='+',
help='Subract one or more masks or region files')
parser.add_argument('--merge_and', dest='mergeand', action='store_true',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I would prefer --merge-and over --merge_and -- but thinking about it another minute, it's not really a "merge" operation, it's an "intersect"... I think the CLI would be cleaner if this was defined as a separate top-level operation rather than a flag on top of "merge":

  parser.add_argument('--merge', dest='merge', metavar="MASK(s)|REG(s)", nargs='+',
                        help='Merge in one or more masks or region files')
  parser.add_argument('--subtract', dest='subtract', metavar="MASK(s)|REG(s)", nargs='+',
                        help='Subract one or more masks or region files')
  parser.add_argument('--intersect', dest='intersect', metavar="MASK(s)|REG(s)", nargs='+',
                        help='Intersect with one or more masks or region files')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just implement explicit, top level logical OR / AND / XOR operations? I think the meaning of "intersect" is less clear. Wrote a simple script for this ages ago and have used it more times than I can count, so it's probably worth adding this general functionality to breizorro...

https://github.com/IanHeywood/oxkat/blob/master/tools/image_logic.py

Copy link
Contributor

@o-smirnov o-smirnov Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set theory language vs. boolean logic language, take your pick...

But then add_argument does allow multiple option names, so why not have it both ways:

parser.add_argument('--merge', '--or', dest='merge', metavar="MASK(s)|REG(s)", nargs='+',
                        help='Merge (logical-OR) one or more masks or region files')
parser.add_argument('--intersect', '--and', dest='intersect', metavar="MASK(s)|REG(s)", nargs='+',
                        help='Intersect (logical-AND) one or more masks or region files')

Just not sure what --subtract is in boolean language? It's not XOR (and is XOR conceivably useful for masks anyway?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think subtract is subtract...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just not sure what --subtract is in boolean language?

NAND? 🤔

is XOR conceivably useful for masks anyway?

Probably not (that's not not NOT) actually.

help='Uses logical AND when merging instead of logical OR - the effect creates ever more restrictive masks')

parser.add_argument('--number-islands', dest='islands', action='store_true', default=False,
help='Number the islands detected (default=do not number islands)')
Expand Down Expand Up @@ -248,11 +252,13 @@ def load_fits_or_region(filename):
fits, regs = load_fits_or_region(merge)
if fits:
LOGGER.info(f"Treating {merge} as a FITS mask")
mask_image += fits[0]
mask_image = np.logical_and(mask_image, fits[0] > 1e-30) \
if args.mergeand else np.logical_or(mask_image, fits[0] > 1e-30)

LOGGER.info("Merged into mask")
else:
LOGGER.info(f"Merging in {len(regs)} regions from {merge}")
add_regions(mask_image, regs, wcs)
add_regions(mask_image, regs, wcs, args.mergeand)
mask_image = mask_image != 0
mask_header['BUNIT'] = 'mask'

Expand Down