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

Issue 1673 import parameters chemistry error #1675

Merged
merged 3 commits into from
Sep 16, 2021

Conversation

brosaplanella
Copy link
Member

Description

This allows importing parameters via chemistry that is in the local directory. I don't think this fixes importing parameters from other folders via chemistry. This is a much more complicated issue (I believe), but this fix should be enough for the workshop.

Fixes #1673

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.

  • 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

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@brosaplanella
Copy link
Member Author

PS: we need to include this fix in the released version for the workshop, so not sure what is the right pipeline for this.

@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #1675 (aa81375) into develop (d5e818a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1675   +/-   ##
========================================
  Coverage    99.27%   99.27%           
========================================
  Files          342      342           
  Lines        18717    18720    +3     
========================================
+ Hits         18582    18585    +3     
  Misses         135      135           
Impacted Files Coverage Δ
pybamm/util.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5e818a...aa81375. Read the comment docs.

@valentinsulzer
Copy link
Member

Let's make a patch release for this. Can you make a PR to main with version 21.8.1?

@brosaplanella
Copy link
Member Author

So, do I merge this to main first with the new version number and then we merge main to develop?

@valentinsulzer
Copy link
Member

Yep, but don't merge this directly to main since it will also merge the other recent changes to develop. You need to make a new branch from main with just these changes. Might be possible with git cherry pick or just do manually

@brosaplanella
Copy link
Member Author

brosaplanella commented Sep 15, 2021

When we switched to calendar versioning we removed the patch number (it is just 21.08 now). Should I bring it in? Technically it is not a new version but just a patch to fix a bug mostly because of the workshop (otherwise we could wait until the next release).

On another point, what should I do with CHANGELOG?

@valentinsulzer
Copy link
Member

Hmm ok yeah change it to 21.08.post2 then (in setup.py), and just add the line to changelog but don't make it a new section

@brosaplanella brosaplanella merged commit 261f6c4 into develop Sep 16, 2021
@brosaplanella brosaplanella deleted the issue-1673-import-parameters-chemistry-error branch September 16, 2021 08:39
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.

Import parameters from chemistry fails when importing from local parameter set
2 participants