-
-
Notifications
You must be signed in to change notification settings - Fork 572
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
Issue 709 simplify on creation #801
Conversation
Codecov Report
@@ Coverage Diff @@
## master #801 +/- ##
==========================================
- Coverage 98.22% 98.14% -0.08%
==========================================
Files 181 179 -2
Lines 10149 9935 -214
==========================================
- Hits 9969 9751 -218
- Misses 180 184 +4
Continue to review full report at Codecov.
|
…am/PyBaMM into issue-709-simplify-on-creation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of small comments
|
||
def __gt__(self, other): | ||
"""return a :class:`Heaviside` object""" | ||
return pybamm.Heaviside(other, self, equal=False) | ||
return pybamm.simplify_if_constant( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having pybamm.simplify_if_constant( )
in every function can we have self = pybamm.simplify_if_constant(self)
in the __init__
of Symbol
? Maybe this screws with the tree though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but python doesn't let you assign self in the __init__
, only its arguments (or at least I couldn't figure out how to), so this seems a bit safer
np.testing.assert_array_equal( | ||
(self.mat @ self.vect).evaluate(), np.array([[5], [2], [3]]) | ||
) | ||
|
||
def test_matrix_modification(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed? I don't see how it is related to simplifying on creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplifying on creation broke the test, then I thought we were never using this functionality so could get rid of the test. We might actually want to prevent matrix modification outright, to avoid weird bugs?
b = pybamm.Scalar(2) | ||
sum = a + b | ||
b = pybamm.Parameter("b") | ||
summ = a + b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again simplify on creation broke the test
Description
Simplify objects as soon as they are created, if they are constant.
Also, removed
update_model
functionality, as this can be more efficiently achieved by usingInputParameter
.Fixes #709
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.
Key checklist:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: