Skip to content
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

Fix boundary condition types #1173

Merged
merged 1 commit into from
Sep 22, 2020
Merged

Conversation

bessman
Copy link
Contributor

@bessman bessman commented Sep 18, 2020

Description

Change boundary condition type in "Creating Models" notebooks, which were incorrectly given as Dirichlet but are in fact Neumann.

Note that this change does not affect the final result of any of the notebooks; the fact that the result is independent of boundary condition type is probably a bug. See also #1174.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ flake8
  • All tests pass: $ python run-tests.py --unit
  • The documentation builds: $ cd docs and then $ make clean; make html

I have one failing test (test_add_rm_param) and one which errors (test_edit_param), but on my system this is the case prior to this PR too.

Copy link
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @bessman!

@brosaplanella
Copy link
Member

Related to the test_add_rm_param error, which PyBaMM version are you using? You might need to reinstall PyBaMM to fix it.

@valentinsulzer
Copy link
Member

Thanks @bessman !

@valentinsulzer valentinsulzer merged commit c885c8e into pybamm-team:develop Sep 22, 2020
brosaplanella added a commit to brosaplanella/PyBaMM that referenced this pull request Sep 22, 2020
@bessman
Copy link
Contributor Author

bessman commented Sep 22, 2020

@ferranbrosa I'm on develop, 87dc41a specifically.

Here are the errors:

test_add_rm_param (test_parameters.test_parameters_cli.TestParametersCLI) ... FAIL
/usr/lib/python3.7/tempfile.py:935: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpleeb5c96'>
  _warnings.warn(warn_message, ResourceWarning)
test_edit_param (test_parameters.test_parameters_cli.TestParametersCLI) ... ERROR
/usr/lib/python3.7/tempfile.py:935: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/home/alexander/projects/pybamm/pybamm/input/parameters/lithium-ion/anodes/tmpkrmqmi1z'>
  _warnings.warn(warn_message, ResourceWarning)
/usr/lib/python3.7/tempfile.py:935: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpcaxjpoav'>
  _warnings.warn(warn_message, ResourceWarning)

...

======================================================================
ERROR: test_edit_param (test_parameters.test_parameters_cli.TestParametersCLI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/alexander/projects/pybamm/tests/unit/test_parameters/test_parameters_cli.py", line 100, in test_edit_param
    with open(copied_path_parameters_file, "r") as f:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpcaxjpoav/lithium-ion/anodes/tmpkrmqmi1z/parameters.csv'

======================================================================
FAIL: test_add_rm_param (test_parameters.test_parameters_cli.TestParametersCLI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/alexander/projects/pybamm/tests/unit/test_parameters/test_parameters_cli.py", line 55, in test_add_rm_param
    self.assertTrue(os.path.isfile(new_parameter_filename))
AssertionError: False is not true

@brosaplanella
Copy link
Member

I think the issue is that when you installed it that command hadn't been added. Because it acts on bash, you need to install it again (no need to clone again or anything, just re-run the installation command). @tlestang is that right?

@bessman bessman deleted the bc-type-fix branch September 22, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants