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

Allow for dynamically computing vectorial properties #114

Closed
bastonero opened this issue Mar 4, 2024 · 8 comments
Closed

Allow for dynamically computing vectorial properties #114

bastonero opened this issue Mar 4, 2024 · 8 comments

Comments

@bastonero
Copy link

The current parameters for LammpsCalculation doesn't allow to specify vectorial compute properties, such as forces (fx, fy, fz) when computing porperty/atom for instance. This would be great to have instead of the LammpsRawCalculation, which is less appealing when used as part of other automated workflows.

@JPchico
Copy link
Collaborator

JPchico commented Apr 15, 2024

Hi @bastonero I've been looking at this, and it turns out that I already had implemented this capability, it was mostly a miss categorization of this property.

The only problem now is to ensure that the formatting is proper, basically if you have a dynamic array in which you do not know the size the line that sets the string format runs into problem. I'm seeing how to fix this, but there is a chance that it might be fixable in an easy way.

@bastonero
Copy link
Author

Thank you very much @JPchico ! Happy to try testing !

JPchico pushed a commit to JPchico/aiida-lammps that referenced this issue Apr 15, 2024
…namic vectors.

Fixing the fact that the property/atoms compute should be a vector of undefined length and not an array.

Fixes aiidaplugins#114
@JPchico
Copy link
Collaborator

JPchico commented Apr 15, 2024

@bastonero If you want you can test with my fork. I tested with the example in the discourse that you put and it seems to work, but please let me know if it works okay now.

Cheers

@bastonero
Copy link
Author

@JPchico thanks a lot, I tried it out and it works (I just have to double check it parses it correctly, but it seems fine)

@JPchico
Copy link
Collaborator

JPchico commented Apr 16, 2024

Great! Let me know if in your other tests it works fine, of that way we can merge this and maybe make a newer release so that the pypi package is up to date.

@JPchico
Copy link
Collaborator

JPchico commented Apr 25, 2024

Hi @bastonero! Are you happy with your tests? I just want to know to see if I can make a PR from my branch to close this issue.

@bastonero
Copy link
Author

Hi @JPchico, I am! The calculations report what I need, so by now I don't anything to report. If something else won't work, I guess can be implemented by a later PR. Thanks a lot!

@JPchico
Copy link
Collaborator

JPchico commented Apr 25, 2024

Fantastic I'll open a PR on this and then when the outstanding PR are merged will make a release.

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

When branches are created from issues, their pull requests are automatically linked.

2 participants