-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: main
Are you sure you want to change the base?
i-pi job update #647
Conversation
Pull Request Test Coverage Report for Build 5047790300
💛 - Coveralls |
@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....
|
So on the scipy requirements, I see the lower pin to 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. |
@liamhuber thank you for getting this sorted! Shall I go ahead and merge this PR? |
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.
@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.
Co-authored-by: Liam Huber <[email protected]>
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 |
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.
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?
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.
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.
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.
Looking at it again, I think start=starts[i] at the beginning of the loop should work.
Good catch! Yeah, that should be perfect
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.
I still have one nit about start = stop
, but otherwise looks better!
PiMD jobs now only collect the beads' centroid positions and forces instead of the positions and forces of all the beads.