[geometry] Rename Transform to AffineTransform

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[geometry] Rename Transform to AffineTransform

Matt Juntunen
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
- 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).

Anyone have any insight or opinions on this? I've created GEOMETRY-80 to track this issue.

Regards,
Matt
Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hello.

Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
<[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
Gilles,

> "AffineMap" (?)

I think any name that doesn't include the word "transform" somehow would probably be confusing.

> 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.

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Tuesday, January 7, 2020 10:29 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hello.

Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
<[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Tuesday, January 7, 2020 10:29 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hello.
>
> Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
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 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.

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Tuesday, January 7, 2020 6:41 PM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Tuesday, January 7, 2020 10:29 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hello.
>
> Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hi.

Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Tuesday, January 7, 2020 6:41 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
> > Sent: Tuesday, January 7, 2020 10:29 AM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >
> > Hello.
> >
> > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> > <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
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?

-Matt


[1] https://en.wikipedia.org/wiki/Geometric_transformation

________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Wednesday, January 8, 2020 8:16 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hi.

Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Tuesday, January 7, 2020 6:41 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
> > Sent: Tuesday, January 7, 2020 10:29 AM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >
> > Hello.
> >
> > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> > <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hi.

Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Wednesday, January 8, 2020 8:16 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
> > Sent: Tuesday, January 7, 2020 6:41 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >
> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
> > <[hidden email]> 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 <[hidden email]>
> > > Sent: Tuesday, January 7, 2020 10:29 AM
> > > To: Commons Developers List <[hidden email]>
> > > Subject: Re: [geometry] Rename Transform to AffineTransform
> > >
> > > Hello.
> > >
> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> > > <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
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?

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. There, we can simply sample the user-defined function as needed and produce matrices that are guaranteed to be affine.

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?

-Matt


________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Monday, January 13, 2020 8:03 PM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hi.

Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Wednesday, January 8, 2020 8:16 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
> > Sent: Tuesday, January 7, 2020 6:41 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >
> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
> > <[hidden email]> 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 <[hidden email]>
> > > Sent: Tuesday, January 7, 2020 10:29 AM
> > > To: Commons Developers List <[hidden email]>
> > > Subject: Re: [geometry] Rename Transform to AffineTransform
> > >
> > > Hello.
> > >
> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> > > <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hello.

2020-01-18 15:40 UTC+01:00, Matt Juntunen <[hidden email]>:

> 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 <[hidden email]>
> Sent: Monday, January 13, 2020 8:03 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
>> Sent: Wednesday, January 8, 2020 8:16 AM
>> To: Commons Developers List <[hidden email]>
>> Subject: Re: [geometry] Rename Transform to AffineTransform
>>
>> Hi.
>>
>> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
>> <[hidden email]> 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 <[hidden email]>
>> > Sent: Tuesday, January 7, 2020 6:41 PM
>> > To: Commons Developers List <[hidden email]>
>> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >
>> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
>> > <[hidden email]> 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 <[hidden email]>
>> > > Sent: Tuesday, January 7, 2020 10:29 AM
>> > > To: Commons Developers List <[hidden email]>
>> > > Subject: Re: [geometry] Rename Transform to AffineTransform
>> > >
>> > > Hello.
>> > >
>> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
>> > > <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
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.

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Saturday, January 18, 2020 1:40 PM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hello.

2020-01-18 15:40 UTC+01:00, Matt Juntunen <[hidden email]>:

> 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 <[hidden email]>
> Sent: Monday, January 13, 2020 8:03 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
>> Sent: Wednesday, January 8, 2020 8:16 AM
>> To: Commons Developers List <[hidden email]>
>> Subject: Re: [geometry] Rename Transform to AffineTransform
>>
>> Hi.
>>
>> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
>> <[hidden email]> 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 <[hidden email]>
>> > Sent: Tuesday, January 7, 2020 6:41 PM
>> > To: Commons Developers List <[hidden email]>
>> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >
>> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
>> > <[hidden email]> 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 <[hidden email]>
>> > > Sent: Tuesday, January 7, 2020 10:29 AM
>> > > To: Commons Developers List <[hidden email]>
>> > > Subject: Re: [geometry] Rename Transform to AffineTransform
>> > >
>> > > Hello.
>> > >
>> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
>> > > <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hi.

Le sam. 18 janv. 2020 à 23:14, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Saturday, January 18, 2020 1:40 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hello.
>
> 2020-01-18 15:40 UTC+01:00, Matt Juntunen <[hidden email]>:
> > 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 <[hidden email]>
> > Sent: Monday, January 13, 2020 8:03 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >
> > Hi.
> >
> > Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
> > <[hidden email]> 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 <[hidden email]>
> >> Sent: Wednesday, January 8, 2020 8:16 AM
> >> To: Commons Developers List <[hidden email]>
> >> Subject: Re: [geometry] Rename Transform to AffineTransform
> >>
> >> Hi.
> >>
> >> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
> >> <[hidden email]> 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 <[hidden email]>
> >> > Sent: Tuesday, January 7, 2020 6:41 PM
> >> > To: Commons Developers List <[hidden email]>
> >> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >> >
> >> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
> >> > <[hidden email]> 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 <[hidden email]>
> >> > > Sent: Tuesday, January 7, 2020 10:29 AM
> >> > > To: Commons Developers List <[hidden email]>
> >> > > Subject: Re: [geometry] Rename Transform to AffineTransform
> >> > >
> >> > > Hello.
> >> > >
> >> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> >> > > <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
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. 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.


> 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.

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Sunday, January 19, 2020 9:06 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hi.

Le sam. 18 janv. 2020 à 23:14, Matt Juntunen
<[hidden email]> 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 <[hidden email]>
> Sent: Saturday, January 18, 2020 1:40 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hello.
>
> 2020-01-18 15:40 UTC+01:00, Matt Juntunen <[hidden email]>:
> > 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 <[hidden email]>
> > Sent: Monday, January 13, 2020 8:03 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >
> > Hi.
> >
> > Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
> > <[hidden email]> 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 <[hidden email]>
> >> Sent: Wednesday, January 8, 2020 8:16 AM
> >> To: Commons Developers List <[hidden email]>
> >> Subject: Re: [geometry] Rename Transform to AffineTransform
> >>
> >> Hi.
> >>
> >> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
> >> <[hidden email]> 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 <[hidden email]>
> >> > Sent: Tuesday, January 7, 2020 6:41 PM
> >> > To: Commons Developers List <[hidden email]>
> >> > Subject: Re: [geometry] Rename Transform to AffineTransform
> >> >
> >> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
> >> > <[hidden email]> 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 <[hidden email]>
> >> > > Sent: Tuesday, January 7, 2020 10:29 AM
> >> > > To: Commons Developers List <[hidden email]>
> >> > > Subject: Re: [geometry] Rename Transform to AffineTransform
> >> > >
> >> > > Hello.
> >> > >
> >> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
> >> > > <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hello.

2020-01-20 14:28 UTC+01:00, Matt Juntunen <[hidden email]>:

> 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 <[hidden email]>
> Sent: Sunday, January 19, 2020 9:06 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le sam. 18 janv. 2020 à 23:14, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
>> Sent: Saturday, January 18, 2020 1:40 PM
>> To: Commons Developers List <[hidden email]>
>> Subject: Re: [geometry] Rename Transform to AffineTransform
>>
>> Hello.
>>
>> 2020-01-18 15:40 UTC+01:00, Matt Juntunen <[hidden email]>:
>> > 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 <[hidden email]>
>> > Sent: Monday, January 13, 2020 8:03 PM
>> > To: Commons Developers List <[hidden email]>
>> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >
>> > Hi.
>> >
>> > Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
>> > <[hidden email]> 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 <[hidden email]>
>> >> Sent: Wednesday, January 8, 2020 8:16 AM
>> >> To: Commons Developers List <[hidden email]>
>> >> Subject: Re: [geometry] Rename Transform to AffineTransform
>> >>
>> >> Hi.
>> >>
>> >> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
>> >> <[hidden email]> 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 <[hidden email]>
>> >> > Sent: Tuesday, January 7, 2020 6:41 PM
>> >> > To: Commons Developers List <[hidden email]>
>> >> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >> >
>> >> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
>> >> > <[hidden email]> 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 <[hidden email]>
>> >> > > Sent: Tuesday, January 7, 2020 10:29 AM
>> >> > > To: Commons Developers List <[hidden email]>
>> >> > > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >> > >
>> >> > > Hello.
>> >> > >
>> >> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
>> >> > > <[hidden email]> 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: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
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 <[hidden email]>
Sent: Monday, January 20, 2020 8:52 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hello.

2020-01-20 14:28 UTC+01:00, Matt Juntunen <[hidden email]>:

> 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 <[hidden email]>
> Sent: Sunday, January 19, 2020 9:06 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] Rename Transform to AffineTransform
>
> Hi.
>
> Le sam. 18 janv. 2020 à 23:14, Matt Juntunen
> <[hidden email]> 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 <[hidden email]>
>> Sent: Saturday, January 18, 2020 1:40 PM
>> To: Commons Developers List <[hidden email]>
>> Subject: Re: [geometry] Rename Transform to AffineTransform
>>
>> Hello.
>>
>> 2020-01-18 15:40 UTC+01:00, Matt Juntunen <[hidden email]>:
>> > 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 <[hidden email]>
>> > Sent: Monday, January 13, 2020 8:03 PM
>> > To: Commons Developers List <[hidden email]>
>> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >
>> > Hi.
>> >
>> > Le lun. 13 janv. 2020 à 04:39, Matt Juntunen
>> > <[hidden email]> 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 <[hidden email]>
>> >> Sent: Wednesday, January 8, 2020 8:16 AM
>> >> To: Commons Developers List <[hidden email]>
>> >> Subject: Re: [geometry] Rename Transform to AffineTransform
>> >>
>> >> Hi.
>> >>
>> >> Le mer. 8 janv. 2020 à 04:39, Matt Juntunen
>> >> <[hidden email]> 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 <[hidden email]>
>> >> > Sent: Tuesday, January 7, 2020 6:41 PM
>> >> > To: Commons Developers List <[hidden email]>
>> >> > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >> >
>> >> > Le mar. 7 janv. 2020 à 17:55, Matt Juntunen
>> >> > <[hidden email]> 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 <[hidden email]>
>> >> > > Sent: Tuesday, January 7, 2020 10:29 AM
>> >> > > To: Commons Developers List <[hidden email]>
>> >> > > Subject: Re: [geometry] Rename Transform to AffineTransform
>> >> > >
>> >> > > Hello.
>> >> > >
>> >> > > Le mar. 7 janv. 2020 à 16:00, Matt Juntunen
>> >> > > <[hidden email]> 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: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hello.

Le lun. 20 janv. 2020 à 16:57, Matt Juntunen
<[hidden email]> a écrit :
>
> 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 was not indicating that the name "EuclideanTransform" would be
better than "AffineTransform", I was wondering about whether the
class itself is redundant.

> I've closed PR #58 and created PR #59 with the latest commits squashed.

I've not looked yet.  But answering below, to hopefully clarify
the misunderstanding.

> > 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.

My understanding is that "Transform" can be documented as:
---CUT---
In Euclidean space, this must be an affine transform.
---CUT---

Gilles

> The current documentation is the most complete and mathematically accurate.
>
> -Matt
>>> [...]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Matt Juntunen
Gilles,

> I was not indicating that the name "EuclideanTransform" would be
> better than "AffineTransform", I was wondering about whether the
> class itself is redundant.

Oh, I misunderstood. The "EuclideanTransform" interface is important because it adds the "applyVector(Vector)" method, which has different behavior than the standard "apply" method. All transforms in the euclidean packages have this method but it is not present in the core Transform because not all spaces have associated Vector types (eg, spherical). I had renamed it AffineTransform in the previous PR not because it exposed new functionality or behavior that made it affine, but because it was located in a module defining an affine space. What do you suggest for the name here?

> My understanding is that "Transform" can be documented as:
> ---CUT---
> In Euclidean space, this must be an affine transform.
> ---CUT---

That's part of what the documentation now says.

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Monday, January 20, 2020 2:28 PM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] Rename Transform to AffineTransform

Hello.

Le lun. 20 janv. 2020 à 16:57, Matt Juntunen
<[hidden email]> a écrit :
>
> 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 was not indicating that the name "EuclideanTransform" would be
better than "AffineTransform", I was wondering about whether the
class itself is redundant.

> I've closed PR #58 and created PR #59 with the latest commits squashed.

I've not looked yet.  But answering below, to hopefully clarify
the misunderstanding.

> > 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.

My understanding is that "Transform" can be documented as:
---CUT---
In Euclidean space, this must be an affine transform.
---CUT---

Gilles

> The current documentation is the most complete and mathematically accurate.
>
> -Matt
>>> [...]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] Rename Transform to AffineTransform

Gilles Sadowski-2
Hello.

2020-01-20 20:39 UTC+01:00, Matt Juntunen <[hidden email]>:

> Gilles,
>
>> I was not indicating that the name "EuclideanTransform" would be
>> better than "AffineTransform", I was wondering about whether the
>> class itself is redundant.
>
> Oh, I misunderstood. The "EuclideanTransform" interface is important because
> it adds the "applyVector(Vector)" method, which has different behavior than
> the standard "apply" method. All transforms in the euclidean packages have
> this method but it is not present in the core Transform because not all
> spaces have associated Vector types (eg, spherical). I had renamed it
> AffineTransform in the previous PR not because it exposed new functionality
> or behavior that made it affine, but because it was located in a module
> defining an affine space. What do you suggest for the name here?

I guess that "EuclideanTransform" is fine (as an extension of the
functionality not the requirements of being "affine").

Regards,
Gilles

>
> [...]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]