Issue 1500 print parameter info by submodel
Description
Further improved on "print_parameter_info" by implementing "by_submodel" feature which allow users to print parameters sub-model wise.
Fixes #1500 and supersedes #3628
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.
- [x] 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:
- [x] 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) - [x] All tests pass:
$ python run-tests.py --all(or$ nox -s tests) - [x] 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
Hello everyone! I was messing around with my forked repo, and one thing led to another, which led to me deleting it from GitHub. I didn't know it would ultimately close the PR I opened. I apologize for this inconvenience; I am still a novice in Git and GitHub, which shows that.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.58%. Comparing base (
8512040) to head (58e10e1).
:exclamation: Current head 58e10e1 differs from pull request most recent head 70ab920. Consider uploading reports for the commit 70ab920 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## develop #3846 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 257 257
Lines 21198 21251 +53
========================================
+ Hits 21110 21163 +53
Misses 88 88
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
'current collector' submodel parameters:
| ==========================================================|=========================================== |
| Parameter | Type of parameter |
| ========================================================= | ========================================== |
| Electrode height [m] | Parameter |
| Electrode width [m] | Parameter |
| Number of electrodes connected in parallel to make a cell | Parameter |
| Current function [A] | FunctionParameter with inputs(s) 'Time[s]' |
Is this better? When I add a new line between the text and the table, it is even harder to see and doesn't look good.
Is this better? When I add a new line between the text and the table, it is even harder to see and doesn't look good.
That does look better; I found this resource, though: https://ozh.github.io/ascii-tables/ which has an exhibition of various tables – and Python by default supports UTF-8, so how about we try the "Unicode (single line)" style from it? Here's how the table would look with that:
'current collector' submodel parameters:
┌───────────────────────────────────────────────────────────┬────────────────────────────────────────────┐
│ Parameter │ Type of parameter │
├───────────────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ Electrode height [m] │ Parameter │
│ Electrode width [m] │ Parameter │
│ Number of electrodes connected in parallel to make a cell │ Parameter │
│ Current function [A] │ FunctionParameter with inputs(s) 'Time[s]' │
└───────────────────────────────────────────────────────────┴────────────────────────────────────────────┘
which looks super readable!
now the tables look like this:
'current collector' submodel parameters:
┌───────────────────────────────────────────────────────────┬────────────────────────────────────────────┐
│ Parameter │ Type of parameter │
├───────────────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ Electrode height [m] │ Parameter │
│ Number of electrodes connected in parallel to make a cell │ Parameter │
│ Electrode width [m] │ Parameter │
│ Current function [A] │ FunctionParameter with inputs(s) 'Time[s]' │
└───────────────────────────────────────────────────────────┴────────────────────────────────────────────┘
I decided not to center the headings because many of the tables were too big and centering these headings didn't help in readability
Check out this pull request on ![]()
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
@agriyakhetarpal, @arjxn-py Is anything left on this one?
The changes on the superseded PR (#3628) were going through code review, so we're waiting on that I guess (tagging @rtimms who had some suggested changes which to me look to have been addressed). IMO the tabular output here looks nice and consistent, better than the previous implementation.
@cringeyburger Can you take care of these minor items. Then once @rtimms approves then we can get this merged
The changes in response to my review look good. I'll do a more thorough re-review once everything else is fixed up. Tag me when it's ready. Thanks for working on this and sorry the review process has taken a while.
@kratman, I have resolved the issues. Please look at them and let me know if anything more is to be done!
Thanks, looks great! The failing tests seem unrelated, let's see what happens after we merge develop.
I think SciPy deprecated something in scipy.sparse, which we'll need to handle on develop itself for Python 3.9 and later (Python 3.8 is okay because SciPy had dropped support for it, so we're using an older version there)
Lychee error is unrelated. I will try re-triggering that one
Might need to trigger again, some of them failed with an internal error
Looks like this is working now. I will squash it soon