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

Modify where number of bond types is chosen from #561

Merged
merged 9 commits into from
Mar 22, 2022

Conversation

raynol-dsouza
Copy link
Contributor

@raynol-dsouza raynol-dsouza commented Mar 15, 2022

As discussed in the pyiron meeting yesterday (14/03/2022), I changed the variable from which the number of bond types is chosen to be written to the structure.inp file.

@coveralls
Copy link

coveralls commented Mar 15, 2022

Pull Request Test Coverage Report for Build 2015615522

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 9 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 70.72%

Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/structure/structurestorage.py 9 86.76%
Totals Coverage Status
Change from base Build 1981206027: 0.3%
Covered Lines: 11900
Relevant Lines: 16827

💛 - Coveralls

@raynol-dsouza
Copy link
Contributor Author

The change seems to be breaking multiple workflows. I will re-open the PR once I have a better idea of where else this bit is used.

@raynol-dsouza raynol-dsouza reopened this Mar 15, 2022
@raynol-dsouza
Copy link
Contributor Author

raynol-dsouza commented Mar 15, 2022

I just double checked that the notebooks that fail in the tests and my local machine are tests_sphinx_sphinx_check_all.ipynb and sqs.ipynb. My changes shouldn't be affecting these notebooks at all...

EDIT: I reverted the changes and confirmed that the change does not break the above mentioned tests.

Copy link
Member

@sudarsan-surendralal sudarsan-surendralal left a comment

Choose a reason for hiding this comment

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

Yeah, the changes you made shouldn't break those notebooks and it has to be something else. But coming back to the bonds issue, it'll be great if you can add the short example you showed on Monday as a unittest just to ensure that any changes in the future doesn't break your jobs. As an example see:

def test_lammps_water(self):

@raynol-dsouza
Copy link
Contributor Author

Thanks for the suggestion! I’ll add a test for this.

@raynol-dsouza raynol-dsouza marked this pull request as draft March 20, 2022 22:47
Copy link
Member

@sudarsan-surendralal sudarsan-surendralal left a comment

Choose a reason for hiding this comment

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

Thanks for the tests. I think a couple of changes would still be necessary

tests/lammps/test_base.py Outdated Show resolved Hide resolved
tests/lammps/test_base.py Outdated Show resolved Hide resolved
Copy link
Member

@sudarsan-surendralal sudarsan-surendralal left a comment

Choose a reason for hiding this comment

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

LGTM :)! Just a minor simplification recommendation

tests/lammps/test_base.py Outdated Show resolved Hide resolved
Co-authored-by: Sudarsan Surendralal <surendralal@mpie.de>
@raynol-dsouza raynol-dsouza marked this pull request as ready for review March 21, 2022 11:21
@sudarsan-surendralal sudarsan-surendralal added the format_black reformat the code using the black standard label Mar 21, 2022
@sudarsan-surendralal sudarsan-surendralal merged commit 3030b77 into master Mar 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the modify_bond_type branch March 22, 2022 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants