Issue 1500 print parameter info by submodel #3846
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ectly on any submodel.
… added by get_coupled_variables in model.variables
… added by get_coupled_variables in model.variables V2
… name as key and its variables as value. Modified `get_parameter_info` to parse through the submodels' variables and give out `parameter_info` in `by_submodel=True` condition Modified `print_parameter_info` to print `parameter_info` of a model with an option to print submodel wise using `by_submodel=True`
…er_info" or "print_parameter_info" directly on a submodel
…er_info" or "print_parameter_info" directly on a submodel V2
…r "print_parameter_info" is used on a submodel.
…info" to get new parameters
…ity and reduced repetition
…ted inside into semi-private methods
…r_info" is used directly on a submodel
|
@agriyakhetarpal, @arjxn-py Is anything left on this one? |
kratman
reviewed
Apr 2, 2024
kratman
reviewed
Apr 2, 2024
kratman
previously requested changes
Apr 2, 2024
|
@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! |
rtimms
approved these changes
Apr 3, 2024
|
Thanks, looks great! The failing tests seem unrelated, let's see what happens after we merge develop. |
|
I think SciPy deprecated something in |
Closed
2 tasks
|
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 |
js1tr3
pushed a commit
to js1tr3/PyBaMM
that referenced
this pull request
Aug 12, 2024
* Add "by_submodel" feature and modify docstring * Added line in CHANGELOG.md * Implemented error handling if `get_parameter_info` method is used directly on any submodel. * Implement changes to fix problem: Keep a track of what variables were added by get_coupled_variables in model.variables * Implement changes to fix problem: Keep a track of what variables were added by get_coupled_variables in model.variables V2 * Created object `self.variables_by_submodel` which contains submodel's name as key and its variables as value. Modified `get_parameter_info` to parse through the submodels' variables and give out `parameter_info` in `by_submodel=True` condition Modified `print_parameter_info` to print `parameter_info` of a model with an option to print submodel wise using `by_submodel=True` * Removed redundant comments. * style: pre-commit fixes * Implemented "NotImplementedError" when user tries to use "get_parameter_info" or "print_parameter_info" directly on a submodel * Implemented "NotImplementedError" when user tries to use "get_parameter_info" or "print_parameter_info" directly on a submodel V2 * Updated Docstring of "get_parameter_info" * Added Test Case for "NotImplementedError" when "get_parameter_info" or "print_parameter_info" is used on a submodel. * Renamed variables, updated test, updated docstring * Added "_find_symbols_by_submodel" method and modified "get_parameter_info" to get new parameters * Optimised "print_parameter_info" by simplification, improved readability and reduced repetition * Removed "calculate_max_lengths" and "format_table_row" from being nested inside into semi-private methods * Tests for "get_parameter_info" for both "by_submodel=False" and "by_submodel=True" * Removed duplicate test in "test_base_submodel" for when "get_parameter_info" is used directly on a submodel * added `test_get_parameter_info_submodel` test using custom model * added `test_print_parameter_info` and `test_print_parameter_info_submodel` * Modified `test_get_parameter_info_submodel` to include edge cases * formatted `test_base_model.py` * Moved change log to unreleased * Changed the formatted table's style * Added `UTF-8` encoding to the format table * Updated jupyter notebook `parameterization.ipynb` and added `UTF-8` encoding in environment variables * added utf-8 encoding in PYBAMM_ENV * Resolve minor issues --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Robert Timms <[email protected]> Co-authored-by: Arjun Verma <[email protected]> Co-authored-by: Eric G. Kratz <[email protected]> Co-authored-by: Agriya Khetarpal <[email protected]> Co-authored-by: cringeyburger <cringeyburger>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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: