-
-
Notifications
You must be signed in to change notification settings - Fork 546
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
Make parameters importable from a directory having "pybamm" in its name #1919
Make parameters importable from a directory having "pybamm" in its name #1919
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1919 +/- ##
========================================
Coverage 99.32% 99.32%
========================================
Files 346 346
Lines 19002 19002
========================================
Hits 18874 18874
Misses 128 128
Continue to review full report at Codecov.
|
Could you add some tests |
For some reason, the tests for the modified line don't work. I tried doing - pybamm.set_logging_level("DEBUG")
import subprocess
# create a new lithium_ion folder in the root PyBaMM directory
subprocess.run(["pybamm_edit_parameter", "lithium_ion"])
# path for a function in the created directory -> x/y/z/PyBaMM/lithium_ion/negative_electrode/ ....
test_path = os.path.join(
os.getcwd(),
"lithium_ion",
"negative_electrodes",
"graphite_Chen2020",
"graphite_LGM50_electrolyte_exchange_current_density_Chen2020.py",
)
# check if the lithium_ion directory is present in PyBaMM -> It is
print(os.listdir(os.getcwd()))
# load the function
func = pybamm.load_function(test_path)
self.assertEqual(
func,
lithium_ion.negative_electrodes.graphite_Chen2020.graphite_LGM50_electrolyte_exchange_current_density_Chen2020.graphite_LGM50_electrolyte_exchange_current_density_Chen2020, # noqa
) This shows me -
Importing parameters from anywhere works completely fine outside of the testing suite. Apparently, importing parameters from anywhere else than |
else: | ||
root_path = filename | ||
|
||
# getcwd() returns "C:\\" when in the root drive and "C:\\a\\b\\c" otherwise | ||
if root_path[0] == "\\" or root_path[0] == "/": | ||
root_path = root_path[1:] |
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.
is "\\"
a single character?
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.
Yes
Yeah this is strange, I get this error locally as well |
CHANGELOG.md
Outdated
@@ -13,7 +13,7 @@ | |||
|
|||
## Bug fixes | |||
|
|||
- Parameters can now be imported from any given path in `Windows` ([#1900](https://github.com/pybamm-team/PyBaMM/pull/1900)) | |||
- Parameters can now be imported from any given path ([#1900](https://github.com/pybamm-team/PyBaMM/pull/1900), [#1919](https://github.com/pybamm-team/PyBaMM/pull/1919)) |
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.
Could you merge develop and move this to the unreleased section
…issue-1918-parameter-path
Did you want to add a test for this? Or shall I merge? |
I found the issue with the failing tests -
|
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.
Thanks!
Description
Check for
pybamm/input/parameters
instead ofpybamm
while trying to import parameters from the base package.Fixes #1918
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:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: