-
-
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 548 variable bc #624
Conversation
…ssue-548-variableBC
…ssue-548-variableBC
Codecov Report
@@ Coverage Diff @@
## master #624 +/- ##
==========================================
+ Coverage 98.5% 98.52% +0.01%
==========================================
Files 173 175 +2
Lines 8776 8861 +85
==========================================
+ Hits 8645 8730 +85
Misses 131 131
Continue to review full report at Codecov.
|
…ssue-548-variableBC # Conflicts: # pybamm/solvers/base_solver.py
does this also have changes from another branch? |
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. Setting d/dt = 0 is a bit ugly and possibly susceptible to drift but done is better than working for now :)
@@ -230,6 +237,25 @@ def options(self, extra_options): | |||
raise pybamm.OptionError( | |||
"thermal effects not implemented for lead-acid models" | |||
) | |||
if options[ | |||
"current collector" | |||
] == "single particle potenetial pair" and not isinstance( |
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.
] == "single particle potenetial pair" and not isinstance( | |
] == "single particle potential pair" and not isinstance( |
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.
not sure why this didn't fail any tests
+ delta_phi_s_p_av | ||
- delta_phi_s_n_av | ||
) | ||
self.algebraic = {i_boundary_cc: v_boundary_cc - local_voltage_expression} |
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.
shouldn't local_voltage_expression
already be in the variables dict (so you don't have to redefine it, introducing potential for a different definition here and in the SPMe class)?
phi_s_cn: pybamm.Scalar(0), | ||
phi_s_cp: param.U_p(param.c_p_init, param.T_ref) | ||
- param.U_n(param.c_n_init, param.T_ref), | ||
i_boundary_cc: applied_current / param.l_y / param.l_z, |
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.
is this the right initial condition for a 1+1D model
solution1.t, | ||
solution1.y, | ||
mesh=mesh, | ||
) |
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.
you could use the helper function "post_process_variables" here
Description
Changes to enable 1 plus 1d and updating state vector between time steps for coupling to pnm
Fixes #548
Type of change
Please delete options that are not relevant.
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: