Skip to content
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

Type of Driving Cycle ('A', 'V', 'W') in Experiment Class #1457

Merged
merged 5 commits into from
Apr 9, 2021

Conversation

alibh95
Copy link
Contributor

@alibh95 alibh95 commented Apr 8, 2021

Description

The experiment.py file is modified to read the operating conditions string which includes the type of driving cycle ('A', 'V', 'W').
This change is suggested by @tinosulzer in #1193.

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

alibh95 and others added 2 commits April 8, 2021 15:27
Modified the experiment.py file to process the type of driving cycle ('A', 'V', 'W') in the operating condition string as suggested by @tinosulzer .
@Saransh-cpp
Copy link
Member

Hi @alibh95, I was also working on this issue and most of the code in this PR is almost the same (even better I'd say) as the one I added in my local files, one addition that I had that is missing here was a value error for the user if the type of data is not specified. Something like this -

raise ValueError(
                    """type of data provided using a drive cycle should be
                    specified using 'A', 'V' or 'W'.
                    For example: {}
                    """.format(
                        examples
                    )
                )

Maybe we can add something like this?

As, for example, if an experiment Run US06 is simulated, the code gives an error -

typ = cond_list[2][1]
IndexError: list index out of range

because of the absence of a specifier like (A)

@alibh95
Copy link
Contributor Author

alibh95 commented Apr 8, 2021

@tinosulzer to create an interpolant in the experiment class, we have to pass the model into it.
As the model is already passed into the simulation class. I think it would be better to create interpolant in that class.

@alibh95
Copy link
Contributor Author

alibh95 commented Apr 8, 2021

@Saransh-cpp I didn't know that you were also working on this part of #1193. That's a great suggestion to raise error if the type of driving cycle is not provided.

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1457 (5264d4c) into develop (dab2451) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1457      +/-   ##
===========================================
+ Coverage    98.27%   98.36%   +0.09%     
===========================================
  Files          280      281       +1     
  Lines        16198    16523     +325     
===========================================
+ Hits         15918    16253     +335     
+ Misses         280      270      -10     
Impacted Files Coverage Δ
pybamm/experiments/experiment.py 100.00% <100.00%> (+2.59%) ⬆️
pybamm/__init__.py 94.91% <0.00%> (-0.05%) ⬇️
pybamm/discretisations/discretisation.py 99.78% <0.00%> (-0.01%) ⬇️
pybamm/solvers/solution.py 100.00% <0.00%> (ø)
pybamm/expression_tree/array.py 100.00% <0.00%> (ø)
pybamm/solvers/casadi_solver.py 100.00% <0.00%> (ø)
pybamm/expression_tree/matrix.py 100.00% <0.00%> (ø)
pybamm/expression_tree/functions.py 100.00% <0.00%> (ø)
pybamm/parameters/parameter_sets.py 100.00% <0.00%> (ø)
pybamm/expression_tree/broadcasts.py 100.00% <0.00%> (ø)
... and 41 more

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 dab2451...5264d4c. Read the comment docs.

@valentinsulzer
Copy link
Member

Ok - good point about where you can define the interpolant. In that case you should also store the time points to pass to the simulation so it can create the interpolant

@alibh95
Copy link
Contributor Author

alibh95 commented Apr 9, 2021

@tinosulzer Kindly review this PR.

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alibh95 , merging

@valentinsulzer valentinsulzer merged commit e025ced into pybamm-team:develop Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants