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

i-pi job update #647

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

i-pi job update #647

wants to merge 13 commits into from

Conversation

raynol-dsouza
Copy link
Contributor

PiMD jobs now only collect the beads' centroid positions and forces instead of the positions and forces of all the beads.

@coveralls
Copy link

coveralls commented Apr 28, 2023

Pull Request Test Coverage Report for Build 5047790300

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 136 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.8%) to 13.317%

Files with Coverage Reduction New Missed Lines %
pyiron_contrib/workflow/has_to_dict.py 4 69.23%
pyiron_contrib/workflow/io.py 12 86.24%
pyiron_contrib/workflow/node.py 13 92.61%
pyiron_contrib/workflow/channels.py 15 89.17%
pyiron_contrib/workflow/workflow.py 16 84.55%
pyiron_contrib/atomistics/mlip/lammps.py 33 0%
pyiron_contrib/atomistics/randspg/randspg.py 43 0%
Totals Coverage Status
Change from base Build 4853665754: 0.8%
Covered Lines: 1655
Relevant Lines: 12428

💛 - Coveralls

@liamhuber
Copy link
Member

@niklassiemer I didn't get to working on our conda problem, but @raynol-dsouza's PR got an even more informative error message than either of us!!! It's late on a Friday for me so I've got to stop working, but I'm super optimistic that we now have enough data to resolve this. It's very weird that our three PRs terminally crashed at such different stages of this error though... if we'd just gotten this error message to start with it would have been much more clear how to proceed....

Could not solve for environment specs
The following packages are incompatible
├─ pyiron_atomistics 0.2.66**  is installable and it requires
│  └─ scipy >=1.10.1  with the potential options
│     ├─ scipy [1.10.1|1.8.0|1.8.1|1.9.0|1.9.1] would require
│     │  └─ python >=3.10,<3.11.0a0 , which conflicts with any installable versions previously reported;
│     ├─ scipy [1.10.1|1.8.0|1.8.1|1.9.0|1.9.1] would require
│     │  └─ python >=3.8,<3.9.0a0 , which conflicts with any installable versions previously reported;
│     ├─ scipy 1.10.1, which can be installed;
│     └─ scipy 1.10.1 would require
│        └─ python >=3.11,<3.12.0a0 , which conflicts with any installable versions previously reported;
└─ scikit-image 0.20.0**  is uninstallable because there are no viable options
   ├─ scikit-image 0.20.0 would require
   │  └─ python >=3.10,<3.11.0a0 , which conflicts with any installable versions previously reported;
   ├─ scikit-image 0.20.0 would require
   │  └─ python >=3.11,<3.12.0a0 , which conflicts with any installable versions previously reported;
   ├─ scikit-image 0.20.0 would require
   │  └─ python >=3.8,<3.9.0a0 , which conflicts with any installable versions previously reported;
   ├─ scikit-image 0.20.0 would require
   │  └─ scipy >=1.8,<1.9.2  but there are no viable options
   │     ├─ scipy [1.10.1|1.8.0|1.8.1|1.9.0|1.9.1], which cannot be installed (as previously explained);
   │     ├─ scipy [1.10.1|1.8.0|1.8.1|1.9.0|1.9.1], which cannot be installed (as previously explained);
   │     └─ scipy [1.8.0|1.8.1|1.9.0|1.9.1] conflicts with any installable versions previously reported;
   └─ scikit-image 0.20.0 conflicts with any installable versions previously reported.
Error: Process completed with exit code 1.

@liamhuber
Copy link
Member

├─ scikit-image 0.20.0 would require
   │  └─ scipy >=1.8,<1.9.2  but there are no viable options

So on the scipy requirements, I see the lower pin to >=1.8, but I don't see the upper pin anywhere....

Ok, that's just because the git head is not 0.20.0 anymore. Indeed, on 0.20.0 the requirement is there.

I'm going to check if 0.21.0 (which only has the lower bound) is available on conda yet... Nope. Not showing on conda releases and there's no PR for it yet on their feedstock.

However, as soon as it's released, bumping our dependency to 0.21.0 should resolve all the currently failing tests.

@raynol-dsouza
Copy link
Contributor Author

@liamhuber thank you for getting this sorted! Shall I go ahead and merge this PR?

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

@raynol-dsouza, lgtm. I have no idea how these jobs actually run or what the rest of their code looks like, but I trust you that it's performing as expected.

I made a few minor comments for (hopefully) improving readability.

pyiron_contrib/atomistics/ipi/ipi_core.py Outdated Show resolved Hide resolved
pyiron_contrib/atomistics/ipi/ipi_core.py Outdated Show resolved Hide resolved
pyiron_contrib/atomistics/ipi/ipi_core.py Outdated Show resolved Hide resolved
pyiron_contrib/atomistics/ipi/ipi_core.py Outdated Show resolved Hide resolved
raynol-dsouza and others added 2 commits May 8, 2023 10:25
Co-authored-by: Liam Huber <liamhuber@greyhavensolutions.com>
temp_list = []
for l in snap_lines[1:]:
temp_list.append([float(n) for n in l.split()[1:]]) # first column is the species. Ignore it.
trajectory.append(temp_list)
start = stop
Copy link
Member

Choose a reason for hiding this comment

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

I still find this formulation really difficult to read. Could you switch it with start = starts[i-1] and move it up to the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

starts[i-1] will become starts[-1] when i=0. I kept it this way to avoid this. Looking at it again, I think start=starts[i] at the beginning of the loop should work.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at it again, I think start=starts[i] at the beginning of the loop should work.

Good catch! Yeah, that should be perfect

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

I still have one nit about start = stop, but otherwise looks better!

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.

None yet

3 participants