-
-
Notifications
You must be signed in to change notification settings - Fork 568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 1500 - Print parameter info by submodel #3628
Closed
cringeyburger
wants to merge
34
commits into
pybamm-team:develop
from
cringeyburger:issue-1500-print-parameter-info-by-submodel
Closed
Changes from 6 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
917a8d1
Add "by_submodel" feature and modify docstring
cringeyburger 58586de
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger d1513b8
Added line in CHANGELOG.md
cringeyburger 699c329
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger 5d8c1fc
Implemented error handling if `get_parameter_info` method is used dir…
cringeyburger e4c05df
Implement changes to fix problem: Keep a track of what variables were…
cringeyburger 27e7554
Implement changes to fix problem: Keep a track of what variables were…
cringeyburger bba8061
Created object `self.variables_by_submodel` which contains submodel's…
cringeyburger 4ae39a5
Removed redundant comments.
cringeyburger 087ba1b
Merge branch 'develop' into issue-1500-print-parameter-info-by-submodel
cringeyburger 6c6b996
style: pre-commit fixes
pre-commit-ci[bot] 5c22f90
Implemented "NotImplementedError" when user tries to use "get_paramet…
cringeyburger f5d5bf7
Implemented "NotImplementedError" when user tries to use "get_paramet…
cringeyburger c643b53
Updated Docstring of "get_parameter_info"
cringeyburger a792f49
Added Test Case for "NotImplementedError" when "get_parameter_info" o…
cringeyburger f0b6401
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger 2a156f7
Renamed variables, updated test, updated docstring
cringeyburger c23a55d
Added "_find_symbols_by_submodel" method and modified "get_parameter_…
cringeyburger d77b7cc
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger 30f66d5
Optimised "print_parameter_info" by simplification, improved readabil…
cringeyburger 78c3785
Removed "calculate_max_lengths" and "format_table_row" from being nes…
cringeyburger b636650
Tests for "get_parameter_info" for both "by_submodel=False" and "by_s…
cringeyburger 42a7fae
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger c7ef815
Removed duplicate test in "test_base_submodel" for when "get_paramete…
cringeyburger 7d3fb83
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger a600580
added `test_get_parameter_info_submodel` test using custom model
cringeyburger f3ad7c4
added `test_print_parameter_info` and `test_print_parameter_info_subm…
cringeyburger 29358f4
Merge branch 'develop' into issue-1500-print-parameter-info-by-submodel
cringeyburger 77a126d
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger be88384
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger f1851e9
Modified `test_get_parameter_info_submodel` to include edge cases
cringeyburger 77bbb0b
Merge branch 'develop' into issue-1500-print-parameter-info-by-submodel
rtimms 70aef8f
Merge branch 'pybamm-team:develop' into issue-1500-print-parameter-in…
cringeyburger ae0737b
Merge branch 'develop' into issue-1500-print-parameter-info-by-submodel
cringeyburger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variables is already a dict with keys corresponding to variable names and values corresponding to their symbols. you'll need to store the coupled variables that each submodel defines in a new object.
i don't know if it makes sense to do this when building the model, or if it should be done separately when getting the parameter info. does it make a difference if we store e.g.
self.variables
andself.variables_by_submodel
where the latter is a dict of dicts (e.g.{"submodel 1": {"var1": x, ...}, ...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I know, there isn't much benefit to make a separate dict. other from keeping info clean and sorted + avoid searching a large dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the latter is better. Accessing elements from a dictionary is usually very fast – even if a dict of dicts is used to print out the information, then it can be "normalised" with the name of a submodel as the key (i.e., print the values of the sub-dictionary, similar to how
pandas
can normalise JSON) to account forby_submodel=True
.If
by_submodel
isFalse
, it can pretty-print the whole dict in a table like it currently does. This method might be better, but would also be harder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.. so what should be done now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add an object in
build_coupled_variables
, e.g.self.variables_by_submodel
which has a structure:The code for this would be the same as before:
This stores both
self.variables
andself.variables_by_submodel
.Next, We can modify
get_parameter_info
to includeby_submodel=False
too.In this way, if we set
print_paramter_info(by_submodel=False)
, thenget_parameter_info
will print the usual way.If we set
print_paramter_info(by_submodel=True)
, then we change theparameter_info
returned byget_parameter_info
by submodel wise.NOTE: I have been coding something for this. I was successful in getting the coupled variables. Now I am experimenting with parameter processing and trying to make the links between
get_parameter_info
andprint_parameter_info
better. Also trying to make the code efficient and cleaner to look at... Will commit soon