Issue 3392 improve documentation #3474
Conversation
|
You should probably remove the "Fixes" line in your description since that keyword is intended to close the issue when the PR is merged. PR #3392 is supposed to be a long running issue, not just for this one line change. Since you put parentheses around the issue number it will not automatically close it, but tou can change it to "Related #3392" to link it to the issue correctly without closing it. |
|
Thank you for your feedback, I have changed fixes to related for now. After completing the second task i can use the Fixes Keyword right? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3474 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 257 257
Lines 20661 20661
========================================
Hits 20576 20576
Misses 85 85 ☔ View full report in Codecov by Sentry. |
|
Is this ready to review or still work in progress? |
|
Hi @brosaplanella , I have done the changes you originally proposed. However while in the discussion on the issue @ejfdickinson proposed to remove detailed documentation for functions which are not suppose to be called by users and replace it with just the permitted |
|
@AbhishekChaudharii If you think the additional changes will take a while, you can get this PR reviewed and merged. The remain changes can be done in a second PR |
pybamm/step/_steps_util.py
Outdated
| @@ -37,7 +37,7 @@ class _Step: | |||
| or "resistance". | |||
| value : float | |||
| The value of the step, corresponding to the type of step. Can be a number, a | |||
| 2-tuple (for cccv_ode), or a 2-column array (for drive cycles) | |||
| 2-tuple (for cccv_ode), or a 2-column array. Can pass list as argument (for drive cycles) | |||
There was a problem hiding this comment.
I don't think this argument can be a list, it has to be a 2-column array.
There was a problem hiding this comment.
Just went to the original issue, the problem is that this comment that the value can be a 2-column array is not shown in the docs because it is not written in steps.py. So I would suggest reverting this change and adding the extra info in the docstrings of steps.py.
|
@brosaplanella I have completed the necessary changes, Can you please review it and if it's good to go, merge this PR so that I can get started on improving docs of User guide or API |
pybamm/step/steps.py
Outdated
| The current value in A. | ||
| Value can be a number, a 2-tuple (for cccv_ode), or a 2-column array (for drive cycles) |
There was a problem hiding this comment.
| The current value in A. | |
| Value can be a number, a 2-tuple (for cccv_ode), or a 2-column array (for drive cycles) | |
| The current value in A. It can be a number or a 2-column array (for drive cycles) |
The cccv_ode is only possible at the _Step level so does not need to be included here.
This applies to all below.
|
@all-contributors please add @AbhishekChaudharii for documentation |
|
I've put up a pull request to add @AbhishekChaudharii! 🎉 |
|
@brosaplanella @kratman Thank you for helping me make my first contribution to open-source |
|
Thank you for contributing, and congratulations on your first contribution! Feel free to take any other unassigned issues if you wish to continue :) |
Description
Added docstring to the print_parameter_info method.
Added information that arrays can be passed as values for drive cycles in Experiment steps
Related #3392
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:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: