Commons Math
  1. Commons Math
  2. MATH-778

Dfp Dfp.multiply(int x) does not comply with the general contract FieldElement.multiply(int n)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0
    • Fix Version/s: 3.1
    • Labels:
      None

      Description

      In class org.apache.commons.math3.Dfp, the method multiply(int n) is limited to 0 <= n <= 9999. This is not consistent with the general contract of FieldElement.multiply(int n), where there should be no limitation on the values of n.

      1. MATH-778.patch
        2 kB
        Thomas Neidhart

        Activity

        Hide
        Thomas Neidhart added a comment -

        Applied patch in r1400671.

        Show
        Thomas Neidhart added a comment - Applied patch in r1400671.
        Hide
        Sébastien Brisard added a comment -

        Seems good to me too. Using shift-right operations would result in faster implementations. I remember having tried that at the time I created this issue, but it seems to me I met with an issue, I can't remember what...
        So for the time being, let's stick with this patch, and once we get some time, we can look into optimizations.

        Show
        Sébastien Brisard added a comment - Seems good to me too. Using shift-right operations would result in faster implementations. I remember having tried that at the time I created this issue, but it seems to me I met with an issue, I can't remember what... So for the time being, let's stick with this patch, and once we get some time, we can look into optimizations.
        Hide
        Luc Maisonobe added a comment -

        The patch seems good to me.

        Show
        Luc Maisonobe added a comment - The patch seems good to me.
        Hide
        Thomas Neidhart added a comment -

        I looked at it as it is one of the few open issues for 3.1. As I am no expert for the Dfp implementation, I did not want to commit it without at least a comment from Sebastien or somebody else who is more proficient in this area than I am.

        Show
        Thomas Neidhart added a comment - I looked at it as it is one of the few open issues for 3.1. As I am no expert for the Dfp implementation, I did not want to commit it without at least a comment from Sebastien or somebody else who is more proficient in this area than I am.
        Hide
        Gilles added a comment -

        Thomas, Sébastien,

        Any reluctance to apply this patch?

        Show
        Gilles added a comment - Thomas, Sébastien, Any reluctance to apply this patch?
        Hide
        Thomas Neidhart added a comment -

        Hi,

        I looked at this issue, and if I understand it correctly, the current multiply(int) method is using a performance shortcut for values of x between 0 and RADIX.

        I did a very simple patch to implement the following logic:

        • if 0<=x<RADIX: call multiplyFast with x
        • otherwise create a new Dfp instance with x and call multiply(Dfp) with it
        Show
        Thomas Neidhart added a comment - Hi, I looked at this issue, and if I understand it correctly, the current multiply(int) method is using a performance shortcut for values of x between 0 and RADIX. I did a very simple patch to implement the following logic: if 0<=x<RADIX: call multiplyFast with x otherwise create a new Dfp instance with x and call multiply(Dfp) with it

          People

          • Assignee:
            Sébastien Brisard
            Reporter:
            Sébastien Brisard
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development