Skip to content

Adds post-process cli guide, specifying vafs by element name#400

Merged
ElliottKasoar merged 15 commits intostfc:mainfrom
harveydevereux:vaf_from_str
Feb 12, 2025
Merged

Adds post-process cli guide, specifying vafs by element name#400
ElliottKasoar merged 15 commits intostfc:mainfrom
harveydevereux:vaf_from_str

Conversation

@harveydevereux
Copy link
Collaborator

I found I could not easily request partial VAFs on the CLI by element names (or StartStopStep likes). But instead would have to write all the indices down, which can be automated in shell but is quite unwieldy. So I implemented selecting vaf atoms by element name. Hopefully a useful addition.

Partial VAFs can be obtained in the CLI like so,

janus md --ensemble nve --struct tests/data/NaCl.cif --steps 100 --traj-every 10\
 --post-process-kwargs "{'vaf_compute': True, 'vaf_atoms': (('Na',), ('Cl',)), 'vaf_output_files': ('vaf_na.dat', 'vaf_cl.dat')}"

where 'Na' and 'Cl' are expanded to indices of those elements in the trajectory.

Also add some documentation for post-processing.

Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

Looks helpful, thanks! The documentation is great!

Added a few minor comments from a quick first look.

Quick question that's related, but not new to this: do you think we may want to rename filter_atoms to atoms_filter? To me, filter_atoms sounds more like a boolean.

Co-authored-by: ElliottKasoar <45317199+ElliottKasoar@users.noreply.github.com>
@harveydevereux
Copy link
Collaborator Author

Looks helpful, thanks! The documentation is great!

Added a few minor comments from a quick first look.

Quick question that's related, but not new to this: do you think we may want to rename filter_atoms to atoms_filter? To me, filter_atoms sounds more like a boolean.

Looks helpful, thanks! The documentation is great!

Added a few minor comments from a quick first look.

Quick question that's related, but not new to this: do you think we may want to rename filter_atoms to atoms_filter? To me, filter_atoms sounds more like a boolean.

I can see that yes so might as well unless anyone can think of a better name

alinelena and others added 4 commits February 6, 2025 15:40
Co-authored-by: ElliottKasoar <45317199+ElliottKasoar@users.noreply.github.com>
Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

I think we might want to handle invalid symbols more nicely. If I ask for "abc" instead of "Cl":

janus md --ensemble nve --struct tests/data/NaCl.cif --steps 100 --traj-every 10 --post-process-kwargs "{'vaf_compute': True, 'vaf_atoms': (('abc',), ('Cl',)), 'vaf_output_files': ('vaf_na.dat', 'vaf_cl.dat')}"

I get TypeError: 'numpy.float64' object is not iterable, which is quite strange.

Copy link
Collaborator Author

@harveydevereux harveydevereux left a comment

Choose a reason for hiding this comment

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

fix user guide typos

harveydevereux and others added 4 commits February 7, 2025 10:05
fix user guide typos

Co-authored-by: ElliottKasoar <45317199+ElliottKasoar@users.noreply.github.com>
@harveydevereux
Copy link
Collaborator Author

I think we might want to handle invalid symbols more nicely. If I ask for "abc" instead of "Cl":

janus md --ensemble nve --struct tests/data/NaCl.cif --steps 100 --traj-every 10 --post-process-kwargs "{'vaf_compute': True, 'vaf_atoms': (('abc',), ('Cl',)), 'vaf_output_files': ('vaf_na.dat', 'vaf_cl.dat')}"

I get TypeError: 'numpy.float64' object is not iterable, which is quite strange.

now checked, and also in tests should recieve

ValueError: {'abc'} not allowed in VAF, allowed symbols are {'Cl', 'Na'}

@ElliottKasoar ElliottKasoar self-requested a review February 7, 2025 16:36
@ElliottKasoar ElliottKasoar merged commit b65c33f into stfc:main Feb 12, 2025
9 checks passed
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.

3 participants