Get function args

[Scottmar93]

Description

A helper function to get information on symbols in a model. Accessed by doing model.info("Negative electrode diffusivity [m2.s-1]"). This then displays class of the symbol associated with that name. If it is a functionparameter it lists the parameters it requires in this model.

Fixes #880

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.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ flake8
• All tests pass: $ python run-tests.py --unit
• The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[valentinsulzer]

There's something a bit jarring about having to pass in two lists that both refer to the same things but one is the object and the other is the string. It might be neater to do something like

pybamm.FunctionParameter("Voltage function [V]", dict_input={"Time [s]": pybamm.t})

Then the FunctionParameter can work out what the input objects and strings are based on the dictionary keys and values

(edit: I wouldn't be against changing FunctionParameter to always have to take in a dictionary input of the above form. I think in general we should strive to only allow one way of doing things)

[rtimms]

this is very useful. do we need the string description of the inputs? or can we just get them from symbol.name? then FunctionParameter can just take the function name and a list of symbols? i think it feels a bit clunky to pass in both the string and symbol. also, not a huge issue, but do people expect to call this from model to get info on the parameters? maybe in the future model.info could be used in a more general way

[Scottmar93]

@tinosulzer I am happy to change to a dict if we feel this is less messy. However, I opted for an optional list as it was the minimal change and definitely wouldn't interfere with any existing functionality (i.e. to_python or casadi and back etc which I didn't want to deal with but maybe this isn't a problem?). I think if we do go for the dict though, that we should enforce it consistently. Thoughts?

@rtimms
Unfortunately we can't use symbol.name as sometimes symbols will be a concatenation or combinations of operated on symbols :(
The logic of using model.info() is that we could in theory define two models with the same function name but which require different inputs. So the form of the function is set by how it is defined in the model.
Yeah, longer-term model.info() could serve to print out some nice summary information of the model. E.g. number of algebraic equations, number of non-algebraic equations, number of parameters, number of variables, etc. Then passing arguments could give you more specific information e.g. model.info(parameter="Negative electrode diffusivity [m2.s-1]"). Would also be really nice to pass in a parameter set and determine whether it is compatible with the model, then print out parameters that the model needs that aren't in the set, but yeah longer term...

[valentinsulzer]

It should be fine because to_python and casadi both act on the already-built expression tree. Agree dict should be used consistently

[codecov]

Codecov Report

Merging #881 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #881      +/-   ##
===========================================
+ Coverage       98%   98.03%   +0.03%     
===========================================
  Files          208      208              
  Lines        10936    11016      +80     
===========================================
+ Hits         10718    10800      +82     
+ Misses         218      216       -2

Impacted Files	Coverage Δ
pybamm/parameters/electrical_parameters.py	100% <ø> (ø)	⬆️
pybamm/parameters/thermal_parameters.py	100% <100%> (ø)	⬆️
...bamm/parameters/standard_parameters_lithium_ion.py	100% <100%> (ø)	⬆️
...ernal_circuit/function_control_external_circuit.py	97.77% <100%> (ø)	⬆️
pybamm/expression_tree/parameter.py	100% <100%> (+2.56%)	⬆️
pybamm/models/base_model.py	98.56% <100%> (+0.59%)	⬆️
...odels/submodels/particle/fickian_many_particles.py	100% <100%> (ø)	⬆️
pybamm/parameters/standard_parameters_lead_acid.py	99.28% <100%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 282ff82...7e66610. Read the comment docs.

[valentinsulzer]

looks good now, thanks, can you fix the coverage?

[valentinsulzer]

nb: time isn't in seconds here, will open separate issue

[Scottmar93]

Happy to remove the seconds if want it to be dimensionless? Or thinking of changing input to be in seconds?

[valentinsulzer]

I think we should do pybamm.t * self.param.timescale so that it is a dimensional time (that is how people will define it externally)

[rtimms]

great, thanks @Scottmar93 ! just a few minor points

[valentinsulzer]

Looks good to me but @rtimms might have more comments