[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16938541#comment-16938541
]
Matt Juntunen commented on GEOMETRY-46:
---
Sounds good to me!
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Attachments: GEOMETRY-46.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16938173#comment-16938173
]
Matt Juntunen commented on GEOMETRY-46:
---
{quote}How?
{quote}
The [original VALJO
blog|https://blog.joda.org/2014/03/valjos-value-java-objects.html] implies that
classes should be final and immutable. Our current implementation isn't final
but it could easily be made to be later on since the constructor is private
along with the only subclass. If we make a public subclass, then we would not
be able to make the {{VectorXD}} classes final if needed.
Overall, though, I could get on board with this design. It would be a nice
touch to be able to explicitly declare parameters and return values as unit
vectors.
I'm picturing this work as being a separate Jira issue. What do you think of
the current PR? Can we merge it and close this issue?
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Attachments: GEOMETRY-46.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16938138#comment-16938138
]
Gilles Sadowski commented on GEOMETRY-46:
-
{quote}breaks ValJO specification
{quote}
How?
{quote}off [...] if Vector2D and Vector3D did things one way and Vector1D did
it another.
{quote}
Of course, the same changes would be applied to those other classes as well.
{quote}What benefits does this design bring?
{quote} # Robustness: Factory method "from" ensures normalization happens in
the class that claims to produce normalized vectors (removes the need for a
comment such as "Callers are responsible for ensuring that the given values
represent a normalized vector"). Also, preconditions are checked within that
same class.
# Explicit representation of a useful concept (cf. basis vectors). Could make
some codes slightly leaner (one never needs to call "normalize" on a {{Unit}}
instance). For example, method "directionTo(v)" should be declared to return a
"Unit" vector. Ditto for method "normalize", of course. ;)
# Static method "normalize(double x)" (and equivalent in 2D and 3D) can be
removed; and this is good IMO for several reasons:
** Mix between vector transformation and factory method (double -> vector).
** Unnecessary addition to the API.
** Asymmetric usage ("of" vs "normalize") in higher level API
("AffineTransformMatrix1D").
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Attachments: GEOMETRY-46.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16937996#comment-16937996
]
Matt Juntunen commented on GEOMETRY-46:
---
I commented on the usage of {{super}} as well :). I've never seen it used that
way before.
I'm not seeing a whole lot of difference in terms of usability or performance
between your patch and the existing version, except for perhaps the {{negate}}
method where you return one of the constants. What benefits does this design
bring?
Here are a couple of other thoughts:
# Having {{Unit}} be public breaks the VALJO specification. We were bending
the specification previously by extending {{VectorXD}} all, but we were using
internal implementation classes that were not part of the public API.
# Would the other {{VectorXD}} classes have changes like this? I'm wondering
because they currently all have a static {{normalize}} method and generally
follow the same patterns for instance creation. It would seem odd to me if
{{Vector2D}} and {{Vector3D}} did things one way and {{Vector1D}} did it
another.
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Attachments: GEOMETRY-46.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16937584#comment-16937584
]
Gilles commented on GEOMETRY-46:
Surprised by this use of {{super}}, I couldn't find any link on the web about
this access to private _instance_ fields (from a _static_ nested class); yet it
works...
However, while looking at that, it occurred to me that there could be some
improvement around the "normalize" functionality. Please comment on the patch
which I've just attached (diff is against PR #39, not master).
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Attachments: GEOMETRY-46.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16937556#comment-16937556
]
Gilles commented on GEOMETRY-46:
In the unit tests, the tolerance should be set to 0 (since the purpose of that
specialized class is to return exactly 1 for the norm).
Nit-pick: Spurious (trailing) white-space characters.
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16937286#comment-16937286
]
Matt Juntunen commented on GEOMETRY-46:
---
This PR is ready to go.
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEOMETRY-46) Additional UnitVector methods
[
https://issues.apache.org/jira/browse/GEOMETRY-46?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16937269#comment-16937269
]
Matt Juntunen commented on GEOMETRY-46:
---
I added some small comments in github on the PR related to style.
Unfortunately, our github build is broken on jdk 13+ so we don't get a green
light on it. [~erans], are you okay with merging anyway?
> Additional UnitVector methods
> -
>
> Key: GEOMETRY-46
> URL: https://issues.apache.org/jira/browse/GEOMETRY-46
> Project: Apache Commons Geometry
> Issue Type: Improvement
>Reporter: Matt Juntunen
>Priority: Minor
> Labels: easyfix, pull-request-available, starter
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The following methods should be overridden in the {{UnitVector}} private
> subclasses of the {{Vector?D}} classes:
> * {{normSq}} -- should return {{1}} for consistency with {{norm}}
> * {{negate}} -- should be overridden to also return a {{UnitVector}} instance
> instead of a regular {{Vector?D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
