commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sébastien Brisard (Commented) (JIRA) <>
Subject [jira] [Commented] (MATH-677) About package "transform"
Date Wed, 30 Nov 2011 11:39:40 GMT


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.

> About package "transform"
> -------------------------
>                 Key: MATH-677
>                 URL:
>             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:!default.jspa
For more information on JIRA, see:


View raw message