Gilles,
> From a design POV, I still think that "AffineTransform" is redundant:
The "AffineTransform" name change has been reverted. It is now the original
"EuclideanTransform". I've closed PR #58 and created PR #59 with the latest
commits squashed.
> IIUC, the required (not just "desired") properties should stand out.
> And, for the mathematically-inclined, the relationship to affine
> transforms would illustrate it (for Euclidean spaces).
I'm not sure what you're saying here. The current documentation is the most
complete and mathematically accurate.
-Matt
________________________________
From: Gilles Sadowski <gillese...@gmail.com>
Sent: Monday, January 20, 2020 8:52 AM
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: [geometry] Rename Transform to AffineTransform
Hello.
2020-01-20 14:28 UTC+01:00, Matt Juntunen <matt.juntu...@hotmail.com>:
> Gilles,
>
>> I had a (quick) look; is it necessary to split functionality among
>> "Transform"
>> (in "core") and its subinterfaces/classes in other modules? IOW, if
>> "Transform"
>> can only be affine, it looks strange to have "AffineTransform"
>> (re)defined.
>
> This is a documentation issue.
> The name "affine transform" only applies to
> affine spaces such as Euclidean space. Spherical space is not an affine
> space. The "Transform" interface is intended to represent transforms with
> the desired properties regardless of whether the space is affine or not.
>From a design POV, I still think that "AffineTransform" is redundant:
* If "Transform" has the "desired properties"
* Then, in an affine space, "Transform" is an affine transform.
> This was not clear in the docs since the word "affine" is listed as an
> implementation requirement on the "Transform" interface. I've updated the
> docs and userguide to clarify this.
IIUC, the required (not just "desired") properties should stand out.
And, for the mathematically-inclined, the relationship to affine
transforms would illustrate it (for Euclidean spaces).
>
>
>> I'm also a bit puzzled by the "AbstractAffineTransformMatrix" that seems
>> to
>> only contain convenience methods for internal use (whereas having them
>> "protected" put them in the public API).
>
> That class also contains other matrix-specific methods (eg, "determinant")
> and the overridden "preservesOrientation". Good point on the protected
> methods, though. I've moved them into the internal "Matrices" utility
> class.
Thanks.
Gilles
>
> -Matt
> ________________________________
> From: Gilles Sadowski <gillese...@gmail.com>
> Sent: Sunday, January 19, 2020 9:06 AM
> To: Commons Developers List <dev@commons.apache.org>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le sam. 18 janv. 2020 à 23:14, Matt Juntunen
> <matt.juntu...@hotmail.com> a écrit :
>>
>> Gilles,
>>
>> >> There, we can simply sample the user-defined function
>> > I'm not sure I understand.
>>
>> Just an implementation detail. We need to pass some sample points through
>> the user-defined function in order to construct an equivalent matrix.
>>
>> > Throwing an exception if the transform does not abide by
>> > the requirements?
>>
>> Yes.
>>
>> I just submitted a PR on Github with these changes. I also realized that
>> the EuclideanTransform class as discussed exactly matches the definition
>> of an affine transform so I renamed it to AffineTransform. No other names
>> were changed.
>
> I had a (quick) look; is it necessary to split functionality among
> "Transform"
> (in "core") and its subinterfaces/classes in other modules? IOW, if
> "Transform"
> can only be affine, it looks strange to have "AffineTransform" (re)defined.
>
> I'm also a bit puzzled by the "AbstractAffineTransformMatrix" that seems to
> only contain convenience methods for internal use (whereas having them
> "protected" put them in the public API).
>
> Regards,
> Gilles
>
>>
>> -Matt
>> ________________________________
>> From: Gilles Sadowski <gillese...@gmail.com>
>> Sent: Saturday, January 18, 2020 1:40 PM
>> To: Commons Developers List <dev@commons.apache.org>
>> Subject: Re: [geometry] Rename Transform to AffineTransform
>>
>> Hello.
>>
>> 2020-01-18 15:40 UTC+01:00, Matt Juntunen <matt.juntu...@hotmail.com>:
>> > Gilles,
>> >
>> >> If the "Transform" is intimately related to the "core" and there is a
>> >> single
>> >> set of properties that make it "affine" (and work correctly), I'd tend
>> >> to
>> >> keep the name "Transform".
>> >
>> > So, if I'm understanding you correctly, you're saying that since the
>> > partitioning code in the library only works with these types of
>> > parallelism-preserving transforms, it can be safely assumed that
>> > o.a.c.geometry.core.Transform represents such a transform. Is this
>> > correct?
>>
>> Indeed.
>>
>> > One thing that's causing some issues with the implementation here is
>> > that
>> > the Euclidean TransformXD interfaces have static
>> > "from(UnaryOperator<X>)"
>> > methods that allow users to wrap their own, arbitrary vector operations
>> > as
>> > Transform instances. We don't (and really can't) do any validation on
>> > these
>> > user-defined functions to ensure that they meet the library
>> > requirements. It
>> > is therefore easy for users to pass in invalid operators. To avoid this,
>> > I'm
>> > thinking of removing the TransformXD interfaces completely and moving
>> > the
>> > "from(UnaryOperator<X>)" methods into the AffineTransformMatrixXD
>> > classes.
>>
>> +1
>> It is generally good to prevent the creation of invalid objects.
>>
>> > There, we can simply sample the user-defined function
>>
>> I'm not sure I understand.
>>
>> > as needed and produce
>> > matrices that are guaranteed to be affine.
>>
>> Throwing an exception if the transform does not abide by
>> the requirements?
>>
>> > Following the above, the class hierarchy would then be as below, which
>> > is
>> > basically what it was before I added the TransformXD interfaces.
>> >
>> > commons-geometry-core
>> > Transform
>> >
>> > commons-geometry-euclidean
>> > EuclideanTransform extends Transform
>> > AffineTransformMatrixXD implements EuclideanTransform
>> > Rotation3D extends EuclideanTransform
>> > QuaternionRotation implements Rotation3D
>> >
>> > commons-geometry-spherical
>> > Transform1S implements Transform
>> > Transform2S implements Transform
>> >
>> > WDYT?
>>
>> +1
>>
>> Best,
>> Gilles
>>
>> >
>> > -Matt
>> >
>> >
>> > ________________________________
>> > From: Gilles Sadowski <gillese...@gmail.com>
>> > Sent: Monday, January 13, 2020 8:03 PM
>> > To: Commons Developers List <dev@commons.apache.org>
>> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >
>> > Hi.
>> >
>> > Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
>> > <matt.juntu...@hotmail.com> a écrit :
>> >>
>> >> Gilles,
>> >>
>> >> > How about keeping "Transform" for the interface name and define a
>> >> > method
>> >> > ... boolean isAffine();
>> >>
>> >> I would prefer to have separate types for each kind of transform.
>> >> This would make the API clear and would avoid numerous checks in the
>> >> code
>> >> in order to see if a particular transform instance is supported. The
>> >> transform types also generally have an "is-a" relationship with each
>> >> other, which seems like a perfect fit for inheritance. [1]
>> >>
>> >> > I don't get that it is an "accuracy" issue. If some requirement is
>> >> > not
>> >> > met,
>> >> results will be plain wrong
>> >>
>> >> Yes, you are correct. I was not very clear in what I wrote. The
>> >> results
>> >> will be completely unusable if the transform does not meet the
>> >> requirements.
>> >>
>> >> > I wonder why the documented requirement that an "inverse transform
>> >> must exist" does not translate into a method ... getInverse();
>> >>
>> >> Good point. All current implementations are able to provide an inverse
>> >> so
>> >> that method should be present on the interface.
>> >>
>> >> In regard to renaming the Transform interface, I had another idea. The
>> >> main purpose of that interface is to provide a way for the
>> >> partitioning
>> >> code in the core module to implement generic transforms of BSP trees
>> >> (see
>> >> AbstractBSPTree.transform()). This is what leads to the requirement
>> >> that
>> >> the transform preserve parallelism, since otherwise, the represented
>> >> region may be warped in such a way as to make the tree invalid.
>> >> However,
>> >> as far as I can tell, there is not a general mathematical term for
>> >> this
>> >> type of transform that applies to Euclidean and non-Euclidean
>> >> geometries.
>> >> So, my thought is to move the Transform interface to the
>> >> "partitioning"
>> >> package to indicate its relationship to those classes and simply name
>> >> it
>> >> something descriptive like "ParallelismPreservingTransform"
>> >> ("parallelism"
>> >> since that is the more generic, non-Euclidean form of the concept of
>> >> "parallel"). The Euclidean module could then provide a true
>> >> "AffineTransform" interface that extends
>> >> "ParallelismPreservingTransform".
>> >> The spherical module transforms would directly inherit from
>> >> "ParallelismPreservingTransform" and thus avoid any incorrect usage of
>> >> the
>> >> term "affine". The class hierarchy would then look like this:
>> >>
>> >> commons-geometry-core
>> >> ParallelismPreservingTransform
>> >>
>> >> commons-geometry-euclidean
>> >> AffineTransform extends ParallelismPreservingTransform
>> >> AffineTransformXD extends AffineTransform
>> >> AffineTransformMatrixXD implements AffineTransformXD
>> >> Rotation3D extends AffineTransform3D
>> >> QuaternionRotation implements Rotation3D
>> >>
>> >> commons-geometry-spherical
>> >> Transform1S implements ParallelismPreservingTransform<Point1S>
>> >> Transform2S implements ParallelismPreservingTransform<Point2S>
>> >>
>> >> I think the type names here are much more descriptive and
>> >> mathematically
>> >> accurate. WDYT?
>> >
>> > I'm not convinced that such a hierarchy would enhance clarity.
>> > If the "Transform" is intimately related to the "core" and there is a
>> > single
>> > set of properties that make it "affine" (and work correctly), I'd tend
>> > to
>> > keep the name "Transform". [As long as unit tests ensure that concrete
>> > implementations possess the expected properties, we are safe.]
>> >
>> > Regards,
>> > Gilles
>> >
>> >> -Matt
>> >>
>> >>
>> >> [1] https://en.wikipedia.org/wiki/Geometric_transformation
>> >>
>> >> ________________________________
>> >> From: Gilles Sadowski <gillese...@gmail.com>
>> >> Sent: Wednesday, January 8, 2020 8:16 AM
>> >> To: Commons Developers List <dev@commons.apache.org>
>> >> Subject: Re: [geometry] Rename Transform to AffineTransform
>> >>
>> >> Hi.
>> >>
>> >> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
>> >> <matt.juntu...@hotmail.com> a écrit :
>> >> >
>> >> > Gilles,
>> >> >
>> >> > > I thought that the question was how to replace "transform"...
>> >> >
>> >> > I should probably clarify. I want to change the name of the
>> >> > Transform
>> >> > class to make it clear that it only represents transforms that
>> >> > preserve
>> >> > parallelism (eg, affine transforms). The most obvious name would be
>> >> > AffineTransform
>> >>
>> >> How about keeping "Transform" for the interface name and define a
>> >> method
>> >> ---CUT---
>> >> /**
>> >> * Move here the doc explaining under what conditions this method can
>> >> return "true".
>> >> */
>> >> boolean isAffine();
>> >> ---CUT---
>> >> ?
>> >>
>> >> Gilles
>> >>
>> >> > like I suggested but I want to make sure that no one objects to this
>> >> > for
>> >> > design or mathematical reasons.
>> >> >
>> >> > > Anyways, what would be the issue(s) of a non-affine transform?
>> >> > > IOW why should implementations of "Transfrom" be restricted to
>> >> > > affine
>> >> > > transform?
>> >> >
>> >> > Instances of Transform are used to transform hyperplanes and
>> >> > hyperplane-bounded regions. If the transform is not affine, then the
>> >> > computed results will not be accurate.
>> >>
>> >> I don't get that it is an "accuracy" issue. If some requirement is not
>> >> met,
>> >> results will be plain wrong; so it depends on usage: when the
>> >> transform
>> >> must be affine, the code being passed that instance should be able to
>> >> check whether it can use it for the intended purpose.
>> >>
>> >> I wonder why the documented requirement that an "inverse transform
>> >> must exist" does not translate into a method
>> >> ---CUT---
>> >> Transform<P> getInverse();
>> >> ---CUT---
>> >>
>> >> Regards,
>> >> Gilles
>> >>
>> >> > -Matt
>> >> > ________________________________
>> >> > From: Gilles Sadowski <gillese...@gmail.com>
>> >> > Sent: Tuesday, January 7, 2020 6:41 PM
>> >> > To: Commons Developers List <dev@commons.apache.org>
>> >> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >> >
>> >> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
>> >> > <matt.juntu...@hotmail.com> a écrit :
>> >> > >
>> >> > > Gilles,
>> >> > >
>> >> > > > "AffineMap" (?)
>> >> > >
>> >> > > I think any name that doesn't include the word "transform" somehow
>> >> > > would probably be confusing.
>> >> >
>> >> > This is supposed to be a synonym.[1]
>> >> > I thought that the question was how to replace "transform"...
>> >> >
>> >> > >
>> >> > > > Was the same "Transform" interface used in both the "euclidean"
>> >> > > > and
>> >> > > > the
>> >> > > "spherical" packages of "Commons Math"?
>> >> > >
>> >> > > Indirectly. SphericalPolygonsSet extended AbstractRegion, which
>> >> > > included an applyTransform(Transform) method.
>> >> >
>> >> > So the "affine" requirement (in the doc) applied there too.
>> >> >
>> >> > Anyways, what would be the issue(s) of a non-affine transform?
>> >> > IOW why should implementations of "Transfrom" be restricted to
>> >> > affine
>> >> > transform?
>> >> >
>> >> > Regards,
>> >> > Gilles
>> >> >
>> >> > [1] https://en.wikipedia.org/wiki/Affine_transformation
>> >> >
>> >> > > -Matt
>> >> > > ________________________________
>> >> > > From: Gilles Sadowski <gillese...@gmail.com>
>> >> > > Sent: Tuesday, January 7, 2020 10:29 AM
>> >> > > To: Commons Developers List <dev@commons.apache.org>
>> >> > > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >> > >
>> >> > > Hello.
>> >> > >
>> >> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
>> >> > > <matt.juntu...@hotmail.com> a écrit :
>> >> > > >
>> >> > > > Hi all,
>> >> > > >
>> >> > > > The documentation for the o.a.c.geometry.core.Transform
>> >> > > > interface
>> >> > > > (which comes from the original commons-math version) states that
>> >> > > > implementations must be affine. Therefore, I think we should
>> >> > > > rename
>> >> > > > this interface to AffineTransform to avoid confusion with other
>> >> > > > types of transforms, such as projective transforms. Potential
>> >> > > > problems with this are
>> >> > > > - the fact that the JDK already has a class with the same name
>> >> > > > (java.awt.geom.AffineTransform), and
>> >> > >
>> >> > > "AffineMap" (?)
>> >> > >
>> >> > > > - I'm not sure if the term "affine" can technically be applied
>> >> > > > to
>> >> > > > non-Euclidean geometries, such as spherical geometry (there may
>> >> > > > be
>> >> > > > nuances there that I'm not aware of).
>> >> > >
>> >> > > Was the same "Transform" interface used in both the "euclidean"
>> >> > > and
>> >> > > the
>> >> > > "spherical" packages of "Commons Math"?
>> >> > >
>> >> > > Regards,
>> >> > > Gilles
>> >> > >
>> >> > > > Anyone have any insight or opinions on this? I've created
>> >> > > > GEOMETRY-80 to track this issue.
>> >> > > >
>> >> > > > Regards,
>> >> > > > Matt
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>> For additional commands, e-mail: dev-h...@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
