-
-
Notifications
You must be signed in to change notification settings - Fork 580
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 580 processed variable #581
Conversation
Codecov Report
@@ Coverage Diff @@
## master #581 +/- ##
==========================================
- Coverage 98.24% 98.22% -0.03%
==========================================
Files 151 151
Lines 7393 7418 +25
==========================================
+ Hits 7263 7286 +23
- Misses 130 132 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 98.54% 98.59% +0.04%
==========================================
Files 169 169
Lines 8441 8450 +9
==========================================
+ Hits 8318 8331 +13
+ Misses 123 119 -4
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, some queries below which should be dealt with first. I also don't think there is any documentation on what auxiliary_domains are? Is this part of Symbol
, if so it should be described here: https://pybamm.readthedocs.io/en/latest/source/expression_tree/symbol.html
Would we need to assign some more attributes to the variables during discretisation? We have the domain/auxiliary_domain, perhaps there should be some logic somewhere that maps between domain/auxiliary_domain and a list of coordinates. Sounds like this should be in the mesh?
Regarding the eval's. They are a bit of a security hole, not that we are terribly concerned with security with modelling packages :) But I think you could get rid of them just using an if statement? So it might be worth doing this to get rid of the error.
…ction in Symbol
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.
These changes look great, thanks @tinosulzer
Description
Change ProcessedVariable to be able to plot variables as functions of t, x and z
This is still pretty hacky but seems to work ok for now.
We will need to think of a better way to do this at some point, perhaps by assigning some more attributes to the variables during discretisation (e.g. whether they depend on x, r or z)
Getting a codacy error with
eval(...)
but can't getast.literal_eval(...)
to work. How bad is it to useeval
?Fixes #580
Type of change
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: