datamininggroup / pfa

Portable Format for Analytics
http://dmg.org/pfa/
27 stars 8 forks source link

Incorrect (at least unexpected) definition for m.ln1p #9

Open jthompson6 opened 8 years ago

jthompson6 commented 8 years ago

The spec for the function m.ln1p defines it as ln(1+ x^2). This is surprising because the normal definition, is ln(1+ x). This aligns with the classic Taylor series (the Mercator series) and java.lang.Math.log1p.

Looking at the code for hadrian, I see that the implementation is actually java.lang.Math.log1p.

I can verify this by running the PFA script

input: double
output: {type: array, items: double}
action:
  - new: [{m.ln1p: [input]}, {m.ln: [{+: [1, input]}]}, {m.ln: [{+: [1, {"**": [input, 2]}]}]}]
    type: {type: array, items: double}

and seeing that the output for the first value matches the second, not the third.

So I think we should change the spec to say ln(1+ x).

N.B. Originally filed as https://github.com/opendatagroup/hadrian/issues/10

jthompson6 commented 8 years ago

ln(1+ x) is also preferable because it allows calculation of ln(1 - small positive number).

wwells commented 8 years ago

Group agrees to update the specification.

jthompson6 commented 8 years ago

I made a pull request to fix it (although I don't know where else (other than Hadrian) that it needs to be updated.

jpivarski commented 7 years ago

This also needs to be updated on http://dmg.org/pfa/docs/library/; generate a new libfcns.html and put it on the website.