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
