Skip to content
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

#1183 added erf and erfc #1184

Merged
merged 8 commits into from
Oct 16, 2020

Conversation

brosaplanella
Copy link
Member

@brosaplanella brosaplanella commented Oct 7, 2020

Description

Added Erf and Erfc functions

Fixes #1183

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)

Key checklist:

  • No style issues: $ flake8
  • All tests pass: $ python run-tests.py --unit
  • The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just needs CHANGELOG :) and you can remove Erfc


def _function_diff(self, children, idx):
""" See :meth:`pybamm.Function._function_diff()`. """
return -2 / np.sqrt(np.pi) * Exponential(-children[0] ** 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this class, given how you defined erfc

@brosaplanella brosaplanella changed the title #1183 added functions and tests #1183 added erf and erfc Oct 7, 2020
@brosaplanella
Copy link
Member Author

Not sure why does the test not pass in MacOs. Maybe the values are slightly different and that's why it crashes. Can someone with a Mac check if that's the case?

@valentinsulzer
Copy link
Member

Yeah it seems like casadi and numpy have different implementations of erf on Mac:

In [5]: casadi.DM(erf(3)) - casadi.erf(3)
Out[5]: DM(-1.11022e-16)

@valentinsulzer
Copy link
Member

Opened casadi/casadi#2672

@brosaplanella
Copy link
Member Author

Should I define an assertCasadiAlmostEqual to test it then?

@valentinsulzer
Copy link
Member

If you can figure out how :) I briefly tried but couldn't. It's still weird that casadi gives different values on mac and windows/linux though

@valentinsulzer
Copy link
Member

CasADi people say it's not something they can fix. So we should figure out how to implement assertCasadiAlmostEqual

(casadi.fabs(casadi.evalf(a) - casadi.evalf(b)) < tol).is_one()
)
else:
self.assertTrue((casadi.fabs(a - b) < tol).is_one())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicely done!

@valentinsulzer valentinsulzer merged commit 9b4c54d into pybamm-team:develop Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add erf and erfc functions
2 participants