-
-
Notifications
You must be signed in to change notification settings - Fork 576
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
Remove .id #1978
Remove .id #1978
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1978 +/- ##
===========================================
- Coverage 99.38% 99.38% -0.01%
===========================================
Files 348 348
Lines 19255 19228 -27
===========================================
- Hits 19136 19109 -27
Misses 119 119
Continue to review full report at Codecov.
|
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.
thanks @tinosulzer !
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. Thanks Valentin!
# e.g. boundary_value_right(cache_123456789) gets replaced with u1(t, 1) | ||
cache_var_id = id_to_julia_variable(var_id, "cache") | ||
cache_var_id = id_to_julia_variable(var.id, "cache") |
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 am a bit confused, why is the .id
here now?
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.
we still need id here to generate names like cache_12345
. I guess it could go into the id_to_julia_variable
function
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.
Ah, I get it. Looks good then!
# e.g. boundary_value_right(cache_123456789) gets replaced with u1(t, 1) | ||
cache_var_id = id_to_julia_variable(var_id, "cache") | ||
cache_var_id = id_to_julia_variable(var.id, "cache") |
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.
Same here
Description
Adds
__eq__
and__hash__
methods forpybamm.Symbol
(possible now we're not using anytree)Replaces
symbol.id
withsymbol
in dictionary keys and for equality checking, where possibleType 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: