Ticket #1668 (deferred enhancement)

Opened 8 years ago

Last modified 6 years ago

copying unmasked pixels to an array

Reported by: jbosch Owned by: becker
Priority: normal Milestone:
Component: afw Keywords:
Cc: rhl, becker, dubcovsky, rowen, bick, cloomis, price, jbosch Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:

not applicable

Description

There has been a long email discussion on this topic already.

Here's a concrete proposal to move forward on this:

  • We reimplement footprintAndMask to do what it should.
  • We add a new function, footprintAndNotMask to do what it did and what multifit needs.
  • We remove intersectMask, since its functionality is now duplicated by footprintAndNotMask and it's not a good name (and applyMask doesn't have an obvious opposite).
  • We rename the FootprintArray header to something else (ideas?).
  • We add a "MaskedFlattener" (name suggestions welcome) class to the FootprintArray header. This is constructed from a mask and can flatten or expand images with the same mask. It's a class rather than a free function so it can store the mask in either pixel or footprint form internally. Which one it will use is an implementation detail. It will be implemented with the footprint code for now, but we will reexamine this choice later.
  • In the post-PT2 future, we plan a class that is just a list of non-overlapping spans ("PixelSet") with no notion of peaks or contiguousness. We probably have what is now the Footprint class contain one of these, and leave discussions of what to actually call these classes and which one current operations will use for the future.

Change History

comment:1 Changed 8 years ago by DefaultCC Plugin

  • Cc rowen, bick, cloomis, price added

comment:2 Changed 8 years ago by jbosch

  • Owner changed from rhl to becker
  • Status changed from new to assigned

comment:3 Changed 8 years ago by becker

  • Status changed from assigned to deferred

Deferred RE Winter/2012 ticket management. Will address with ip_diffim ticket #1756.

comment:4 follow-up: ↓ 5 Changed 6 years ago by rhl

  • Cc jbosch added

This is a mess. The function footprintAndMask was originally supposed to give the intersection of a Footprint with a Mask, but that was inverted when it was renamed to intersectMask and implemented (let's not try to sort out the history).

Should these be free functions that return new Footprints? I'm in two minds here, but lean towards saying, "No". Unfortunately we cannot use the name intersectMask. How about:

  • Add void Footprint::andNotMask(Mask const& mask, int bitmask=~0) which is the current functionality of intersectMask
  • Write void Footprint::andMask(Mask const& mask, int bitmask=~0) which returns to its original functionality under a newish name
  • Write 'std::shared_ptr<Footprint> Footprint::clone() (which allows f = foot.clone(); f.andMask(mask) to recover the ability to make a new Footprint) (it could return Footprint or a std::unique_ptr` instead).

The bitmask argument tells the code which bits in the mask to pay attention to (default: all).

If there is enough agreement, this should become its own ticket. We have to do something about intersectMask in Winter2013 as it is positively confusing.

comment:5 in reply to: ↑ 4 Changed 6 years ago by jbosch

Replying to rhl:

This is a mess. The function footprintAndMask was originally supposed to give the intersection of a Footprint with a Mask, but that was inverted when it was renamed to intersectMask and implemented (let's not try to sort out the history).

Should these be free functions that return new Footprints? I'm in two minds here, but lean towards saying, "No".

I'm ok with either approach, but I think with the implementations of this we've had so far, there's no performance benefit to having in-place version; I think the implementation basically ends up writing a new set of spans and swapping it in.

Unfortunately we cannot use the name intersectMask. How about:

  • Add void Footprint::andNotMask(Mask const& mask, int bitmask=~0) which is the current functionality of intersectMask
  • Write void Footprint::andMask(Mask const& mask, int bitmask=~0) which returns to its original functionality under a newish name
  • Write 'std::shared_ptr<Footprint> Footprint::clone() (which allows f = foot.clone(); f.andMask(mask) to recover the ability to make a new Footprint) (it could return Footprint or a std::unique_ptr` instead).

The bitmask argument tells the code which bits in the mask to pay attention to (default: all).

Sounds reasonable. I'd be equally happy with methods that returned new Footprints, as I think that's the most common use case, and as I mentioned above, no less efficient. Either way, Fpotprint should probably have a virtual clone() method anyhow, because it actually is a base class in a hierarchy (HeavyFootprint is a subclass), so a true clone() is something you can't just do by invoking the copy constructor.

If there is enough agreement, this should become its own ticket. We have to do something about intersectMask in Winter2013 as it is positively confusing.

Fine with me. I had ambitious plans to do a bigger overhaul of Footprint vs. Mask operations back in W12 that got derailed by the higher-priority afw::table work (it got design-reviewed and everything), and while I'd like to resurrect that someday, this is a clear improvement that we can do much faster and without breaking much else.

I also think we don't need to have a terribly long deprecation period for intersectMask before we just remove it; I don't think it's used in many places. Perhaps just leave it in until Mario gets the new Buildbot up and running?

comment:6 Changed 6 years ago by becker

I'd also generally prefer that methods return new Footprints, e.g. you would like to compare the original and masked pixel data associated with the Footprint. And agree that current intersectMasks could be replaced in short order.

Note: See TracTickets for help on using tickets.