Issue 3392 improve documentation #3474

AbhishekChaudharii · 2023-10-26T14:19:41Z

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.

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: $ 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)
All tests pass: $ python run-tests.py --all (or $ nox -s tests)
The documentation builds: $ 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:

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

kratman · 2023-10-27T01:18:59Z

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.

AbhishekChaudharii · 2023-10-27T06:56:48Z

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 · 2023-10-27T08:59:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e802b11) 99.58% compared to head (0aa0358) 99.58%.
Report is 20 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

brosaplanella · 2023-11-07T14:49:41Z

Is this ready to review or still work in progress?

AbhishekChaudharii · 2023-11-07T15:43:06Z

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 value formats and kwargs. I haven't done that in this PR since I feel the current detailed documentation for the internal code will help assist future developers or maintainers to understand and debug the code

kratman · 2023-11-07T15:53:20Z

@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

brosaplanella · 2023-11-09T13:57:28Z

pybamm/step/_steps_util.py

@@ -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)


I don't think this argument can be a list, it has to be a 2-column array.

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.

AbhishekChaudharii · 2023-11-30T06:36:20Z

@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

brosaplanella

Looks better, just a small comment.

brosaplanella · 2023-12-04T15:12:31Z

pybamm/step/steps.py

        The current value in A.
+        Value can be a number, a 2-tuple (for cccv_ode), or a 2-column array (for drive cycles)


Suggested change

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.

brosaplanella

Looks good thanks!

brosaplanella · 2023-12-06T09:02:41Z

@all-contributors please add @AbhishekChaudharii for documentation

allcontributors · 2023-12-06T09:02:50Z

@brosaplanella

I've put up a pull request to add @AbhishekChaudharii! 🎉

AbhishekChaudharii · 2023-12-07T12:51:04Z

@brosaplanella @kratman Thank you for helping me make my first contribution to open-source☺️

brosaplanella · 2023-12-08T09:15:06Z

Thank you for contributing, and congratulations on your first contribution! Feel free to take any other unassigned issues if you wish to continue :)

* Added docstring for print_parameter_info method * PEP8 adherence for One-line docstring * Mentioned that arrays can be passed as values for drive cycles. * reverted changes from 3c59897 * value can be a 2-column array added in steps.py * style: pre-commit fixes * removed cccv_ode * style: pre-commit fixes * removed cccv_ode - fix --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

AbhishekChaudharii added 2 commits October 26, 2023 18:56

Added docstring for print_parameter_info method

7aee06e

PEP8 adherence for One-line docstring

be87f81

Mentioned that arrays can be passed as values for drive cycles.

3c59897

AbhishekChaudharii closed this Nov 7, 2023

AbhishekChaudharii reopened this Nov 7, 2023

brosaplanella reviewed Nov 9, 2023

View reviewed changes

AbhishekChaudharii and others added 3 commits November 29, 2023 13:18

reverted changes from 3c59897

9e82990

value can be a 2-column array added in steps.py

7c5995f

style: pre-commit fixes

53efe92

AbhishekChaudharii requested a review from brosaplanella November 29, 2023 12:50

brosaplanella reviewed Dec 4, 2023

View reviewed changes

AbhishekChaudharii and others added 4 commits December 5, 2023 16:17

Merge branch 'pybamm-team:develop' into issue_3392_improve_documentation

ffd36dd

removed cccv_ode

efba2e0

style: pre-commit fixes

6b6ad0d

removed cccv_ode - fix

0aa0358

AbhishekChaudharii requested a review from brosaplanella December 5, 2023 11:19

brosaplanella approved these changes Dec 6, 2023

View reviewed changes

allcontributors bot mentioned this pull request Dec 6, 2023

docs: add AbhishekChaudharii as a contributor for doc #3594

Merged

brosaplanella merged commit 4fa8933 into pybamm-team:develop Dec 6, 2023
35 checks passed

AbhishekChaudharii deleted the issue_3392_improve_documentation branch December 8, 2023 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 3392 improve documentation #3474

Issue 3392 improve documentation #3474

AbhishekChaudharii commented Oct 26, 2023 •

edited

Loading

kratman commented Oct 27, 2023

AbhishekChaudharii commented Oct 27, 2023

codecov bot commented Oct 27, 2023 •

edited

Loading

brosaplanella commented Nov 7, 2023

AbhishekChaudharii commented Nov 7, 2023

kratman commented Nov 7, 2023

brosaplanella Nov 9, 2023

brosaplanella Nov 9, 2023

AbhishekChaudharii commented Nov 30, 2023

brosaplanella left a comment

brosaplanella Dec 4, 2023

brosaplanella left a comment

brosaplanella commented Dec 6, 2023

allcontributors bot commented Dec 6, 2023

AbhishekChaudharii commented Dec 7, 2023

brosaplanella commented Dec 8, 2023

		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.
	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)

Issue 3392 improve documentation #3474

Issue 3392 improve documentation #3474

Conversation

AbhishekChaudharii commented Oct 26, 2023 • edited Loading

Description

Type of change

Key checklist:

Further checks:

kratman commented Oct 27, 2023

AbhishekChaudharii commented Oct 27, 2023

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

brosaplanella commented Nov 7, 2023

AbhishekChaudharii commented Nov 7, 2023

kratman commented Nov 7, 2023

brosaplanella Nov 9, 2023

Choose a reason for hiding this comment

brosaplanella Nov 9, 2023

Choose a reason for hiding this comment

AbhishekChaudharii commented Nov 30, 2023

brosaplanella left a comment

Choose a reason for hiding this comment

brosaplanella Dec 4, 2023

Choose a reason for hiding this comment

brosaplanella left a comment

Choose a reason for hiding this comment

brosaplanella commented Dec 6, 2023

allcontributors bot commented Dec 6, 2023

AbhishekChaudharii commented Dec 7, 2023

brosaplanella commented Dec 8, 2023

AbhishekChaudharii commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading