PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Issue 1500 print parameter info by submodel

Open cringeyburger opened this issue 2 years ago • 6 comments

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

cringeyburger avatar Feb 28 '24 13:02 cringeyburger

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.

cringeyburger avatar Feb 28 '24 13:02 cringeyburger

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.

codecov[bot] avatar Feb 28 '24 15:02 codecov[bot]

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

cringeyburger avatar Feb 28 '24 19:02 cringeyburger

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!

agriyakhetarpal avatar Feb 28 '24 19:02 agriyakhetarpal

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

cringeyburger avatar Feb 28 '24 20:02 cringeyburger

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@agriyakhetarpal, @arjxn-py Is anything left on this one?

kratman avatar Apr 02 '24 14:04 kratman

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.

agriyakhetarpal avatar Apr 02 '24 14:04 agriyakhetarpal

@cringeyburger Can you take care of these minor items. Then once @rtimms approves then we can get this merged

kratman avatar Apr 02 '24 15:04 kratman

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.

rtimms avatar Apr 02 '24 15:04 rtimms

@kratman, I have resolved the issues. Please look at them and let me know if anything more is to be done!

cringeyburger avatar Apr 02 '24 22:04 cringeyburger

Thanks, looks great! The failing tests seem unrelated, let's see what happens after we merge develop.

rtimms avatar Apr 03 '24 08:04 rtimms

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)

agriyakhetarpal avatar Apr 03 '24 10:04 agriyakhetarpal

Lychee error is unrelated. I will try re-triggering that one

kratman avatar Apr 04 '24 13:04 kratman

Might need to trigger again, some of them failed with an internal error

agriyakhetarpal avatar Apr 04 '24 13:04 agriyakhetarpal

Looks like this is working now. I will squash it soon

kratman avatar Apr 04 '24 14:04 kratman