-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
[Bug]: pybamm.Simulation.set_parameters
, pybamm.Simulation.set_up_and_parameterise_experiment
and pybamm.Simulation.set_up_and_parameterise_model_for_experiment
made private in simulation.py
#3752
Changes from 5 commits
5917f2a
f4188c1
4aa147c
ad30142
a6848ee
40d152d
6ae475d
86a7056
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ | |||||
from datetime import timedelta | ||||||
from pybamm.util import have_optional_dependency | ||||||
from typing import Optional | ||||||
from warnings import warn | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicking here, but we should keep this uniform throughout the codebase -
Suggested change
and then warnings.warn everywhere |
||||||
|
||||||
from pybamm.expression_tree.operations.serialise import Serialise | ||||||
|
||||||
|
@@ -177,6 +178,7 @@ def _set_random_seed(self): | |||||
|
||||||
def set_up_and_parameterise_experiment(self): | ||||||
""" | ||||||
This is a helper function. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
Set up a simulation to run with an experiment. This creates a dictionary of | ||||||
inputs (current/voltage/power, running time, stopping condition) for each | ||||||
operating condition in the experiment. The model will then be solved by | ||||||
|
@@ -185,6 +187,8 @@ def set_up_and_parameterise_experiment(self): | |||||
This needs to be done here and not in the Experiment class because the nominal | ||||||
cell capacity (from the parameters) is used to convert C-rate to current. | ||||||
""" | ||||||
msg = "pybamm.simulation.set_up_and_parameterise_experiment is not meant to be accessed directly." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
warn(msg, DeprecationWarning) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that not a lot of users are using these methods and we want to make them private in the next release - we could create dummy public methods that throw def set_up_and_parameterise_experiment(self):
"""
A note here for users that this should not be used and is not private
with the name _set_up_and_parameterise_experiment
"""
raise NameError(...) Just suggestions. We should go ahead with the strategy that most maintainers agree with. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. happy to take suggestions and do what is decided upon There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's a good idea to make them private right in the next release, since it doesn't conform to our policy of three releases – even if the popularity of this API not being relatively high is the question. Keeping them public plus raising a DeprecationWarning should be enough, a NameError sounds a bit odd IMO (I don't think we have used this before). |
||||||
# Update experiment using capacity | ||||||
capacity = self._parameter_values["Nominal cell capacity [A.h]"] | ||||||
for op_conds in self.experiment.operating_conditions_steps: | ||||||
|
@@ -209,12 +213,15 @@ def set_up_and_parameterise_experiment(self): | |||||
|
||||||
def set_up_and_parameterise_model_for_experiment(self): | ||||||
""" | ||||||
This is a helper function. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
Set up self._model to be able to run the experiment (new version). | ||||||
In this version, a new model is created for each step. | ||||||
|
||||||
This increases set-up time since several models to be processed, but | ||||||
reduces simulation time since the model formulation is efficient. | ||||||
""" | ||||||
msg = "pybamm.simulation.set_up_and_parameterise_model_for_experiment is not meant to be accessed directly." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
warn(msg, DeprecationWarning) | ||||||
self.experiment_unique_steps_to_model = {} | ||||||
for op_number, op in enumerate(self.experiment.unique_steps): | ||||||
new_model = self._model.new_copy() | ||||||
|
@@ -325,7 +332,8 @@ def set_parameters(self): | |||||
""" | ||||||
A method to set the parameters in the model and the associated geometry. | ||||||
""" | ||||||
|
||||||
msg = "pybamm.set_paramters is meant to be accessed directly." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
warn(msg, DeprecationWarning) | ||||||
if self.model_with_set_params: | ||||||
return | ||||||
|
||||||
|
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'll also need to shift it to the unreleased section