New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add erf function #234

Merged
merged 15 commits into from Sep 22, 2017

Conversation

Projects
None yet
2 participants
@benjello
Contributor

benjello commented May 16, 2017

This PR adds an erf function using scipy.

@gdementen

This comment has been minimized.

Member

gdementen commented May 16, 2017

I do not want to depend on scipy, so the function must be either optional (only available if scipy is installed) or computed via an expression like shown at http://stackoverflow.com/questions/457408/is-there-an-easily-available-implementation-of-erf-for-python, if the later approach gives sufficient precision.

@benjello

This comment has been minimized.

Contributor

benjello commented May 16, 2017

@gdementen : what is your favorite implementation ?
I would choose an optional scipy as a standard practice but the function you gave may be faster
and sufficient for liam2 needs.

@gdementen

This comment has been minimized.

Member

gdementen commented May 16, 2017

I have no idea if what I linked to is precise enough. I am not really qualified to tell. I would accept either, so do what you prefer/think is best. The second option might be faster indeed thanks to numexpr/multi-threading, but that would need to be tested.

@gdementen

This comment has been minimized.

Member

gdementen commented May 16, 2017

I cannot find to code of the erf function in scipy, it seems like it links to a C implementation from some other lib, so it is unlikely we will get any faster, at least with a single thread.

@benjello

This comment has been minimized.

Contributor

benjello commented May 17, 2017

There is a link to the implementation in C on this page.

I will use the scipy version.

@benjello

This comment has been minimized.

Contributor

benjello commented May 17, 2017

If the implementation suits you I can even write a line in the changelog ;-)

@benjello

This comment has been minimized.

Contributor

benjello commented Sep 21, 2017

@gdementen : any chance this get merged soon ?

@gdementen

Sorry for the awful delay. It completely went off my radar. If you could add a note in the documentation & changelog, that would be great.

def compute(self, context, expr):
if scipy is None:
raise ImportError(
"Scipy was not succesfully imported",

This comment has been minimized.

@gdementen

gdementen Sep 22, 2017

Member

I would like a better message for people trying to use it without scipy installed. Something like:
"Failed to import scipy, which is required for erf(). Please make sure scipy is installed and working."

@benjello

This comment has been minimized.

Contributor

benjello commented Sep 22, 2017

Needs to be done:

  • changelog
  • note in documentation
@benjello

This comment has been minimized.

Contributor

benjello commented Sep 22, 2017

@gdementen : rebased, doc and changelog added

@gdementen gdementen merged commit e8fc667 into liam2:master Sep 22, 2017

2 checks passed

code-quality/landscape Code quality decreased by -0.20%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gdementen

This comment has been minimized.

Member

gdementen commented Sep 22, 2017

Thanks

@benjello benjello deleted the benjello:add_erf_function branch Sep 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment