[
https://issues.apache.org/jira/browse/IMAGING-228?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16855122#comment-16855122
]
Bruno P. Kinoshita edited comment on IMAGING-228 at 6/3/19 10:42 PM:
---------------------------------------------------------------------
Hi [~erans]
Glad I created a PR and this issue instead of committing directly, some review
& feedback is always appreciated.
>Why would you do that?
For maintainability - for now.
>Just saw the commit on GitHub: The original code is how I would have done it.
>;) Intuitively, I'd bet that the call to {{Math.pow}} is slower...
Agree, even if a few ms or ns, this is probably some performance users would
appreciate, as parsing images is not always the fastest operation. But at the
moment the old code base needs some polishing, and IMO it could do with
simplification list this, even at cost of performance.
This `#cube` method, for example, is not the only one in Imaging. There's
another one in
[ColorConversion|[https://github.com/apache/commons-imaging/blob/eb98398bd111cdc35b2c9a5fc8022a28d7c99035/src/main/java/org/apache/commons/imaging/color/ColorConversions.java#L536].]
ColorConversion has also a `#square` method.
But if you look at ColorConversion, even though it has a `#cube` method, it
also uses Math.pow(N, 3).
We would either have to maintain both, or start creating some sort of utility
class to accommodate these methods. And I would prefer doing this later, in the
whole project, and considering whether this should be maintained here or if
there are libraries out there that could help us (e.g. I remember commons-math
had some sort of optimized math methods, that could outperform the ones from
JDK?).
So my plan is to use Math methods for now, then revisit performance later in
another ticket, and try to do it right across the whole project. What do you
think?
was (Author: kinow):
Hi [~erans]
Glad I created a PR and this issue instead of committing directly, some review
& feedback is always appreciated.
>Why would you do that?
For maintainability - for now.
>Just saw the commit on GitHub: The original code is how I would have done it.
>;) Intuitively, I'd bet that the call to {{Math.pow}} is slower...
Agree, even if a few ms or ns, this is probably some performance users would
appreciate, as parsing images is not always the fastest operation. But at the
moment the old code base needs some polishing, and IMO it could do with
simplification list this, even at cost of performance.
This `#cube` method, for example, is not the only one in Imaging. There's
another one in
[ColorConversion|[https://github.com/apache/commons-imaging/blob/eb98398bd111cdc35b2c9a5fc8022a28d7c99035/src/main/java/org/apache/commons/imaging/color/ColorConversions.java#L536].]
ColorConversion has also a `#square` method.
But if you look at ColorConversion, even though it has a `#cube` method, it
also uses Math.pow(N, 3). And even though it has `#cube` it also uses Math.sqrt.
We would either have to maintain both, or start creating some sort of utility
class to accommodate these methods. And I would prefer doing this later, in the
whole project, and considering whether this should be maintained here or if
there are libraries out there that could help us (e.g. I remember commons-math
had some sort of optimized math methods, that could outperform the ones from
JDK?).
So my plan is to use Math methods for now, then revisit performance later in
another ticket, and try to do it right across the whole project. What do you
think?
> Remove private method PhotometricInterpreterLogLuv#cube by Math.pow
> -------------------------------------------------------------------
>
> Key: IMAGING-228
> URL: https://issues.apache.org/jira/browse/IMAGING-228
> Project: Commons Imaging
> Issue Type: Improvement
> Reporter: Bruno P. Kinoshita
> Assignee: Bruno P. Kinoshita
> Priority: Minor
> Fix For: 1.0-alpha2
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> We have a private method in PhotometricInterpreterLogLuv, that calculates the
> cube of a number N doing N * N * N. That can be replaced by Math.pow(N, 3).
> Will add unit tests and some javadocs around photometric interpreters,
> logluv, cei, lab, color spaces, ceixyz-ceilab, etc, as well.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
