Hi,
I've noticed the matrix branch has been merged on master as is,
without squashing commits, and without a review.
That's rather uncommon (if not unique for something a proposal based
set of changes), and I still believe a review is important, so I'm going
to do one, please treat it as if it was done against the pull
request we missed.
Since we lost Github support for reviews, I'm going to just list my
observations
inline.
---------------
I've seen that many of the classes changed did not receive
copyright header updates (some, like GeneralMatrix, did instead).
Here is, for reference, the list of files that have been changed, and whose
copyright headers might have to be fixed:
> git diff --name-only 82bb0f144604e9d9f04da49304b87c82e687216f..HEAD
.gitignore
docs/user/images/matrix2.png
docs/user/images/matrix2.ucls
modules/library/metadata/src/main/java/org/geotools/math/Line.java
modules/library/metadata/src/main/java/org/geotools/math/Plane.java
modules/library/metadata/src/main/java/org/geotools/math/Statistics.java
modules/library/metadata/src/test/java/org/geotools/math/GeometryTest.java
modules/library/opengis/pom.xml
modules/library/referencing/pom.xml
modules/library/referencing/src/main/java/org/geotools/referencing/datum/BursaWolfParameters.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/DefaultCoordinateOperationFactory.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/AdvancedAffineBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/AffineTransformBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/BursaWolfTransformBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/MathTransformBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/ProjectiveTransformBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/RubberSheetBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/builder/SimilarTransformBuilder.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/GeneralMatrix.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/Matrix1.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/Matrix2.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/Matrix3.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/Matrix4.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/SingularMatrixException.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/XMatrix.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/transform/AbstractMathTransform.java
modules/library/referencing/src/main/java/org/geotools/referencing/operation/transform/ProjectiveTransform.java
modules/library/referencing/src/test/java/org/geotools/referencing/operation/builder/MathTransformBuilderTest.java
modules/library/referencing/src/test/java/org/geotools/referencing/operation/matrix/GeneralMatrixTest.java
modules/library/referencing/src/test/java/org/geotools/referencing/operation/transform/GeocentricTransformTest.java
modules/plugin/epsg-hsql/src/test/java/org/geotools/referencing/factory/epsg/OperationFactoryTest.java
modules/plugin/imagemosaic-jdbc/pom.xml
modules/plugin/imagemosaic/pom.xml
modules/unsupported/wfs-ng/src/test/java/org/geotools/data/wfs/integration/v1_1/GeoServerIntegrationTest.java
pom.xml
spike/jan/gsoc-transformations/pom.xml
---------------
https://github.com/geotools/geotools/blob/master/modules/library/metadata/src/main/java/org/geotools/math/Plane.java#L49
The Point3D class are amassed without vertical spacing (coding convention
breakage), none of public methods
have javadocs. Unsure why the class is nested, the class seems general and
it is being used without a Plane around, e.g.:
https://github.com/geotools/geotools/blob/master/modules/library/referencing/src/test/java/org/geotools/referencing/operation/transform/GeocentricTransformTest.java#L185
---------------
Confused about the new hashcode business in GeneralMatrix:
https://github.com/geotools/geotools/blob/matrix/modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/GeneralMatrix.java#L584
Issues:
* This does not seem like a valid duplicate of the original hash code of
vecmath, that used all matrix elements (this one will result in a
accumulation
of all matrices having the same top/left element in the same bucket).
For reference, the original one:
https://java.net/projects/vecmath/sources/svn/content/trunk/src/javax/vecmath/GMatrix.java?rev=144
* The code is duplicated in a static method a few lines below:
https://github.com/geotools/geotools/blob/master/modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/GeneralMatrix.java#L613
This static method is then called to build the hashcode of Matrix3, but
I don't see the reason, aren't we inheriting the GeneralMatrix one anyways?
Same goes for equals with tolerance in Matrix3, seems that we should be
inheriting the base class one
Here I seem to see an optimization for nothing:
https://github.com/geotools/geotools/blob/matrix/modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/GeneralMatrix.java#L840
It seems the original intention was to leverage an in-place operation, to
avoid instantiating another DenseMatrix64F, but then the implementation
does create one regardless and replaces mat (so the original test has been
done for nothing, or not?)
The sub public methods lack a javadoc:
https://github.com/geotools/geotools/blob/master/modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/GeneralMatrix.java#L876
----------------
Typo in Matrix1, "matirx":
https://github.com/geotools/geotools/blob/master/modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/Matrix1.java#L163
----------------
Matrix2 has been marked as deprecated, yet Eclipse still finds 22
references to it. If you deprecate a class it would be
good to clean the source code of its usage, no?
Same goes for Matrix3 and Matrix4... wondering if the deprecation notes
should just be removed?
Otherwise we are just getting into the Filter business again, years and
years of deprecated classes without a (funded) plan to remove them.
Matrix has a "determinant", not a "determinate":
https://github.com/geotools/geotools/blob/master/modules/library/referencing/src/main/java/org/geotools/referencing/operation/matrix/Matrix2.java#L212
About Matrix3 and Matrix4, it seems that vecmath had special high
performance classes for the 3 and 4 cases, while
here we have replaced them with a general construct instead, so we are
going to pay array checks for all operations.
Not awfully worried, the cases in which we do a ton of computations are for
coverage reprojection, where we tend to replace the full math
with a localized affine equivalent that's coded in JAI, but ... I thought
you were going to expect a speedup, by what I see, maybe we'll get a small
slowdown instead?
That's all folks. :-)
Cheers
Andrea
--
==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.
==
Ing. Andrea Aime
@geowolf
Technical Lead
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*
Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.
The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.
-------------------------------------------------------
------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
GeoTools-Devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
