[
https://issues.apache.org/jira/browse/MATH-677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13159988#comment-13159988
]
Sébastien Brisard commented on MATH-677:
----------------------------------------
Hi Gilles,
thanks for your advice. So I'll confine to the cosmetic changes, and we will
postpone the rest to the next major release. I'm a regular user of FFTs, and
I've been involved in debugging JTransforms, as well as setting up the whole
set of unit tests for this library. In short, I am quite willing to dive deeper
into this package... when we get some time.
Regarding point 4, I like your solution, because one class would correspond to
one unique pair {{transform}}/{{inverseTransform}}, with no risk to invoke
{{transform}} followed by {{inverseTransform2}} (and vice-versa).
However, if I may be the devil's advocate, I have a very small objection (but I
can live with that). {{transform}} and {{transform2}} actually perform the
*same* transform, only with a different prefactor. But the transform is
essentially the same (see the implementation).
On the other hand, there are other *types* of DCTs/DSTs on the market. The
currently implemented DCT is in fact DCT-I. It would be nice at some point to
have in CM DCT-II, DCT-III, ... which would be different classes. But then, we
would need classes for DCT-Ia, DCT-Ib, ..., DCT-IIa, DCT-IIb, ... for different
normalizing factors. It might be a bit complicated, and would put on the same
logical level *variants* of the same type of DCT, with different types of DCTs,
which I'm slightly (but only very slightly) uncomfortable with.
To sum up: the two versions of DCT currently implemented in the same class are
essentially the same up to a scaling factor. I dislike the existence of
{{transform2}}/{{inverseTransform2}}, but I think version 2 of the DCT should
be performed by the same class. That's the reason why I was proposing factory
methods (say {{createVersion1()}}/{{createVersion2()}}. Different classes would
be reserved to different versions of the DCT/DST.
Again, these thoughts are only for the sake of arguing, I'm quite happy with
any solution anyone would prefer.
Sébastien
> About package "transform"
> -------------------------
>
> Key: MATH-677
> URL: https://issues.apache.org/jira/browse/MATH-677
> Project: Commons Math
> Issue Type: Improvement
> Reporter: Gilles
> Assignee: Sébastien Brisard
> Priority: Minor
> Labels: api-change
> Fix For: 3.0
>
>
> Classes in package "o.a.c.m.transform" might require some changes in order to
> conform to goals set for the next major release.
> Some observations:
> # Exceptions
> ## Should remove use of deprecated "MathRuntimeException"
> ## Should throw more specific "Math...Exception" instances instead of
> standard IAE
> # Interface "RealTransformer" (and implementations) contain non-conformant
> method names (e.g. "inversetransform" instead of "inverseTransform").
> {color:red}Fixed in {{r1208293}}.{color}
> # "FastFourierTransformer":
> ## Methods "mdfft" and "verifyDataSet" take an argument of type "Object" (to
> allow an argument with an unspecified number of dimensions)
> ## The "RootsOfUnity" helper class could be moved to the "complex" package
> ## For clarity, multidimensional transform should be moved to a class of its
> own (and I also wonder whether the "MultiDimensionalComplexMatrix" name is
> not misleading)
> # "FastFourierTransformer", "FastSineTranformer" and "FastCosineTranformer"
> define public methods "tranform2" and "inversetransform2" but they are not
> part of an interface
> # Code uses variables that start with an uppercase
> # "FastHadamardTransformer" contains illegible developer documentation (see
> Javadoc for protected method "fht")
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
