Issue 920 dimensional interp #1028
Conversation
valentinsulzer
left a comment
valentinsulzer
left a comment
There was a problem hiding this comment.
Thanks very much for doing this @rtimms ! Some comments here are requested changes and some are just comments for discussion
Clearly there are a few different ways of accessing the time variable. I think the recommended way should be sol["Time [s]"].entries
examples/scripts/SPMe_SOC.py
Outdated
| time = sol["Time [h]"] | ||
| # Coulomb counting | ||
| time_hours = time(sol.t) | ||
| time_secs = sol.t * model.timescale_eval |
There was a problem hiding this comment.
maybe do sol["Time [s]"].entries to avoid having to call model.timescale_eval?
There was a problem hiding this comment.
agreed that the recommended way should be sol["Time [s]"].entries. I've made this consistent everywhere (I think)
pybamm/solvers/processed_variable.py
Outdated
| except AttributeError: | ||
| if self.warn: | ||
| pybamm.logger.warning("No timescale provided. Using default of 1 [s]") | ||
| self.timescale = 1 |
There was a problem hiding this comment.
A model should always have a timescale, since the base model has the default timescale of 1s
There was a problem hiding this comment.
there is a bit of an issue with the shape when the timescale depends on the inputs. In base_solver the timescale is evaluated with the initial inputs. if you then try to do model.timescale.evaluate(inputs=solution.inputs) in ProcessedVariable then the entries of inputs are now of length t. For now I've done a try/except with using timescale_eval and then evaluating if that isn't found. Another option would be to store the initial values of the inputs in the solution and use that (since the timescale should be the same for all times). Or to go through inputs and only use the first entry.
Not sure what the best approach is. Thoughts?
There was a problem hiding this comment.
I think the safest option is to not allow the timescale to depend on inputs. i.e. do model.timescale.evaluate() instead of model.timescale.evaluate(inputs=solution.inputs) (and use a try except for a clear error message). Allowing the timescale to change in e.g. parameter fitting runs is probably asking for trouble
There was a problem hiding this comment.
yeah I agree. I've added a check for this in base_solver
pybamm/solvers/processed_variable.py
Outdated
| self.spatial_vars = {} | ||
| if solution.model: | ||
| for var in ["x", "y", "z", "r_n", "r_p"]: | ||
| if var and var + " [m]" in solution.model.variables: |
There was a problem hiding this comment.
this doesn't check that both var and var + " [m]" are in solution.model.variables. It just checks that var is not None or False, and that var + " [m]" is in solution.model.variables. You have to check both separately.
pybamm/solvers/processed_variable.py
Outdated
| t = self.t_sol * self.timescale | ||
| elif self.base_variable.is_constant(): | ||
| t = self.t_sol[0] | ||
| t = self.t_sol[0] * self.timescale |
There was a problem hiding this comment.
Am wondering whether t_sol should be stored in seconds. I guess you are either consistent with sol.t, or consistent with first_dim_pts, but you can't have both
There was a problem hiding this comment.
Yeah I went back and forth on this. I've added an attribute t_pts which is dimensional time. That way things ending _sol are the dimensionless values and things ending _pts are dimensional (and are the values used in the interpolant)
There was a problem hiding this comment.
Great, that makes sense!
| "Using default of 1 [m]".format(name) | ||
| ) | ||
| scale = 1 | ||
| return scale |
There was a problem hiding this comment.
maybe models should have a r_scale, x_scale, etc in the same way that they have a timescale. Probably a separate issue though. It would simplify processed variables and quickplot a lot.
There was a problem hiding this comment.
Yeah this would definitely be the neatest solution. I've made a new issue for this.
| processed_var = pybamm.ProcessedVariable(var_sol, pybamm.Solution(t_sol, y_sol)) | ||
| processed_var = pybamm.ProcessedVariable( | ||
| var_sol, pybamm.Solution(t_sol, y_sol), warn=False | ||
| ) |
There was a problem hiding this comment.
Oh, I see, solution doesn't have a model here for timescale. Maybe Solution.model could be initialized to BaseModel?
There was a problem hiding this comment.
yep, this worked thanks!
valentinsulzer
left a comment
valentinsulzer
left a comment
There was a problem hiding this comment.
Happy with this now, thanks!
Description
Calls to
ProcessedVariableobjects are made using dimensional time and spaceFixes #920
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 docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: