Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Standardize invert and getInverse to inverted

Description

geom::LinearTransform and geom::AffineTransform both use invert to return an inverted copy of the transform, which sounds like it modifies the transform in place. afw Transform and astshim Mapping use getInverted to return an inverted copy of the transform, which is clearer but a bit verbose.

I would like to standardize all of these to inverted, which is clear and concise and matches other modern usage in our code, e.g. sphgeom.

I would also like to rename simplify to simplified in astshim for clarity and consistency (this is not widely used and would be a very small change).

The changes proposed would require small changes to roughly a dozen packages. I'm happy to do the work. If folks want less churn then a smaller scale version of the proposal is to switch inverse to getInverted (and simplify to getSimplified, for consistency).

Checklist

Lucidchart Diagrams

Issue Matrix

hide

Activity

Show:
Russell Owen
July 20, 2018 at 8:16 PM

Adopted as stated

Jim Bosch
July 19, 2018 at 8:57 AM

I believe that rule only says we shouldn't use get<Noun> for something that may do a lot of work; invert is itself a verb, so it doesn't need get at all.  The rule this conforms to is relatively recent:

https://developer.lsst.io/cpp/style.html#a-names-for-methods-that-return-new-objects-may-start-with-past-tense-verbs

Russell Owen
July 19, 2018 at 8:54 AM

returning an inverted transformation is trivial for afw Transform and astshim Mapping, and I believe it is also trivial for AffineTransform and LinearTransform (though can correct me if not). simplify is also trivial in astshim.

Paul Price
July 19, 2018 at 8:47 AM

I thought our style guide would require calculateInverse, as it includes a verb which communicates to the user that the operation may not be not trivial.

Jim Bosch
July 17, 2018 at 10:19 AM

👍.

 

As a (mostly) implementation note, I think it would be healthy for us to actually leave the old APIs available and mark them as deprecated, even if we update all downstream usage immediately - I'd like us to start getting practice with changing our APIs in a way that's more conducive to versioning different [groups of] packages separately.  That doesn't have to be this RFC, but I think this one is in a sweet spot of "used enough, but not too much" downstream to make it a good practice target.

Done
Pinned fields
Click on the next to a field label to start pinning.

Details

Assignee

Reporter

Planned End

Components

Checklist

Created July 17, 2018 at 10:04 AM
Updated August 12, 2018 at 4:35 PM
Resolved August 12, 2018 at 4:35 PM