DM-13905: Change SpherePoint.getVector to return a sphgeom UnitVector3d

[r-owen]

No description provided.

[TallJimbo]

Looks good; just a few micro-comments.

[TallJimbo]

Should be able to remove some of these includes.

[TallJimbo]

To be consistent with the C++, you probably want to repeat this with the opposite order (this just makes SpherePoint implicitly convertible to LonLat, not vice versa).

[TallJimbo]

Seems like a good candidate for inlining.

[TallJimbo]

It might be worthwhile to just add using sphgeom::Vector3d; within afw::geom in a public header.

[r-owen]

I'd rather hold off adding anything to lsst::afw::geom until we at least move most of it into lsst::geom. Even then I'm a bit leery. I guess I can see how it makes it easier to construct SpherePoints from vectors.

[kfindeisen]

I'm a bit concerned by the growing number of SpherePoint constructors -- we're up to six, not including the two copy-constructors. The sheer number of combinations is going to make the SpherePoint API hard to learn; why not keep a small number of "obviously different" constructors and let the user convert as necessary?

[r-owen]

I share your concern but I am not comfortable removing any.

The LonLat constructor makes LonLat implicitly convertible to SpherePoint in a way that avoids adding dependencies to sphgeom. This seems a useful feature as we integrate more of our code with sphgeom. There is too much overlap between afw geom and sphgeom, but that's not a wart we are likely to be able to fix any time soon, if ever. The best we can do for now is to try to make them interoperate seamlessly.

The sphgeom::UnitVector3d constructor seems like clutter to me, but it avoids the need to normalize the argument so I reluctantly think it worth doing. It also seems a logical constructor to have.

The sphgeom::Vector3d constructor is one I would be loath to give up, as we make heavy use of it.

Which ones would you give up?

[TallJimbo]

For me, the big downside of having too many constructors is just the amount of overloading. One approach that may make everyone happy would be to make some of them static member functions. The implicit conversions would need to stay as constructors, of course, but maybe construction from Vector3d and UnitVector3d could move to a pair of overloaded fromVector methods?

[r-owen]

We have a fair amount of code that constructs a SpherePoint from a vector. So this would be a rather expensive change. On the other hand, the packages requiring change are probably already on this ticket.

I was actually wondering if sphgeom::UnitVector3d and SpherePoint should be implicitly convertible to each other. They are equivalent ways of expressing a point on a sphere. (But that would only make sense if sphgeom::LonLat and sphgeom::UnitVector3d were also implicitly convertible).

How strongly do you feel about this? I personally would rather keep things as they are, as I feel all the constructors make sense. But if you both think things are getting too cluttered you are probably right.

[kfindeisen]

While it is an option, I'm a bit wary of relying on factory methods as replacements for constructors because my experience (particularly with Gaia) is that most astronomers find them unintuitive. A more generally applicable objection is that documentation systems don't highlight factory methods in any way, so they're harder to find (basically, you've replaced the problem of deciding which constructor you want with the problem of deciding which methods you want).

As for what could be trimmed, (Angle, Angle), (double, double, AngleUnit), and (LonLat) convey basically the same information; I'd be inclined to remove one of the first two. I'm not convinced implicit conversions from LonLat are a good idea, but having a dedicated constructor for LonLat at least prevents users from mixing up longitude and latitude if that's a common conversion.

Likewise, (Vector3d) and (UnitVector3d) are redundant. We could keep just (Vector3d) on the grounds that it's more frequently used. I don't think the minor efficiency advantage of (UnitVector3d) is worth the clutter. If we later add implicit conversion of UnitVector3d to Vector3d (which seems a reasonable operation), then an overload that takes both (Vector3d) and (UnitVector3d) would be ambiguous anyway.

I don't like the zero-argument constructor, but I think that discussion is already closed.

[kfindeisen]

I'd vote against implicit conversions between UnitVector3d and SpherePoint, because not all unit vectors are intended to represent points on a sphere.

[r-owen]

I agree with @kfindeisen's objections to fromVector. Another argument is that LonLat can be constructed directly from a vector and LonLat and SpherePoint are very similar.

That said, I will note one point in favor of fromVector: it would be a natural way to add a SpherePoint constructor that takes 3 floats: (x, y, z). This would be one of the most heavily used constructors in Python, second only to (lonAsFloat, latAsFloat, angleUnit), and it would save the bother of doing this: SpherePoint(sphgeom.Vector3d(*values)). Indeed in Python I suspect SpherePoint(Vector3d) would hardly ever be used if (x, y, z) was available, which would save importing any symbols from sphgeom in most Python code. (And yes that is also an argument in favor of making Vector3d available in lsst::afw::geom:: and lsst.afw.geom, as @TallJimbo suggested)

SpherePoint(float, float, angle) is far too heavily used to consider removing.

SpherePoint(angle, angle) is worth considering but I am against doing so because it feels like the most obvious constructor, and because it would greatly expand the scope of this ticket for what I see as very little gain.

I propose the following:

• Remove SpherePoint(unitVector3d), as per @kfindeisen's suggestion
• Remove SpherePoint(LonLat) for now and make the implicit conversion one way (SpherePoint to LonLat). We can always add it if and when we find a need. We have no identified use case, though it may well become a need when we use sphgeom package more heavily.

[TallJimbo]

I think I'd cut that down to just removing SpherePoint(unitVector3d); when a naive user in the future asks, "why is SpherePoint convertible to LonLat but not vice versa?", I think "we felt SpherePoint had too many constructors" will feel like a poor excuse.

[kfindeisen]

After discussion with @r-owen, we've agreed that keeping SpherePoint(LonLat) is best. He pointed out that creating a SpherePoint from a LonLat using the vector- or angle-based constructors is actually pretty hard because sphgeom doesn't use afw::geom types anywhere.