DM-34721: miscellaneous cleanups and union/intersection regions

[TallJimbo]

No description provided.

[fritzm]

You might consider a resize() here and then just using array subscripting assignments, to avoid the redundant bounds checking in eight successive push_back() calls. OTOH, there are less than ten of them, so it's probably a shrug :-)

[TallJimbo]

I'm a little worried it could be an accidental pessimization, because the pattern is that lots of functions like this are going to be pushing into the same buffer vector, and I don't actually know if resize is going to (when the capacity isn't big enough) allocate just the amount of extra space needed or do the kind of "allocate more than you did last time" thing that keeps push_back amortized constant-time. Do you happen to know?

[fritzm]

Good point. I think you could call capacity() before resize() to avoid the undesired behavior, but I have also just realized the the resize() is going to have to do a default init on all eight slots, so that will have a cost as well.

In any case, for eight items per batch I think it's hard to argue convincingly that there would be much of a win or loss here in the end.

[fritzm]

typo: "they we"

[fritzm]

What about cases where neither r1 nor r2 individually contain rhs, but the union of r1 and r2 does (for example, two overlapping circles that together contain an ellipse not entirely contained by either one individually?)

[TallJimbo]

You're absolute right about the behavior, but we're saved by the limited guarantees made by the Region interface on the behavior; from the docs on Region::relate:

    /// Said another way: if the CONTAINS, WITHIN or DISJOINT bit is set, then
    /// the corresponding spatial relationship between the two regions holds
    /// conclusively. If it is not set, the relationship may or may not
    /// hold.
    ///
    /// These semantics allow for conservative relationship computations. In
    /// particular, a Region may choose to implement `relate` by replacing
    /// itself and/or the argument with a simplified bounding region.

It'd be nice to do better, but I think we'd at least need way for all Region classes to be able to compute some common set of shapes they bound (rather than are bounded by), and that's a lot of hard math for still not being able to formally guarantee any more than we do.

[fritzm]

I think there are also cases where neither operand is individually within rhs, but the intersection of the operands is (consider two overlapping circles, and rhs an ellipse that surrounds the intersection but extends outside both of the circles)

[TallJimbo]

(other thread on related problem for unions applies here, too)

[fritzm]

This looks like a nice feature to have. Some questions in comments about whether all cases are properly covered in the relate() methods?

Additionally interesting: compound regions are the first possibly non-convex regions in this library. At first I wondered if this might violate some assumptions elsewhere in the library (e.g. in various pixelization codes) but I didn't see any issues with this offhand...

[TallJimbo]

compound regions are the first possibly non-convex regions in this library. At first I wondered if this might violate some assumptions elsewhere in the library (e.g. in various pixelization codes) but I didn't see any issues with this offhand

That's a good question, but I don't have a strong enough understanding of what "convex" means for general regions on the sphere to tell if it is qualitatively new in that respect. It certainly does permit non-convex great-circle polygons, which we didn't before, but we could already define Box and Circle objects (those covering more than a hemisphere) with non-great-circle boundaries that would look more concave than convex in some projections (but I'm waving my hands, because they also don't have straight-line edges in any projection I can reason intuitively about).