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

name ML properties in the results file by architecture, energy become… #176

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

alinelena
Copy link
Member

@alinelena alinelena commented May 31, 2024

…s mace_energy,...

  • tests

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.

At the moment this seems to write out things twice:

Lattice="5.62 0.0 0.0 0.0 5.62 0.0 0.0 0.09808252417753258 5.619144046778919" 
Properties=species:S:1:pos:R:3:spacegroup_kinds:I:1:mace_mp_forces:R:3:forces:R:3
spacegroup="P 1" 
unit_cell=conventional
occupancy="_JSON {\"0\": {\"Na\": 1.0}, \"1\": {\"Cl\": 1.0}, \"2\": {\"Na\": 1.0}, \"3\": {\"Cl\": 1.0}, \"4\": {\"Na\": 1.0}, \"5\": {\"Cl\": 1.0}, \"6\": {\"Na\": 1.0}, \"7\": {\"Cl\": 1.0}}" 
mace_mp_energy=-27.020331256158105
mace_mp_stress="-0.006830042553032722 -0.006875546081961216 -0.006926724236709431 0.0012606249302715168 -1.3664263946131713e-19 3.979321394392908e-19"
energy=-27.020331256158105 
stress="-0.006830042553032722 3.979321394392908e-19 -1.3664263946131713e-19 3.979321394392908e-19 -0.006875546081961216 0.0012606249302715168 -1.3664263946131713e-19 0.0012606249302715168 -0.006926724236709431"
free_energy=-27.020331256158105
pbc="T T T"

Looks mostly ok otherwise

@alinelena
Copy link
Member Author

At the moment this seems to write out things twice:

Lattice="5.62 0.0 0.0 0.0 5.62 0.0 0.0 0.09808252417753258 5.619144046778919" 
Properties=species:S:1:pos:R:3:spacegroup_kinds:I:1:mace_mp_forces:R:3:forces:R:3
spacegroup="P 1" 
unit_cell=conventional
occupancy="_JSON {\"0\": {\"Na\": 1.0}, \"1\": {\"Cl\": 1.0}, \"2\": {\"Na\": 1.0}, \"3\": {\"Cl\": 1.0}, \"4\": {\"Na\": 1.0}, \"5\": {\"Cl\": 1.0}, \"6\": {\"Na\": 1.0}, \"7\": {\"Cl\": 1.0}}" 
mace_mp_energy=-27.020331256158105
mace_mp_stress="-0.006830042553032722 -0.006875546081961216 -0.006926724236709431 0.0012606249302715168 -1.3664263946131713e-19 3.979321394392908e-19"
energy=-27.020331256158105 
stress="-0.006830042553032722 3.979321394392908e-19 -1.3664263946131713e-19 3.979321394392908e-19 -0.006875546081961216 0.0012606249302715168 -1.3664263946131713e-19 0.0012606249302715168 -0.006926724236709431"
free_energy=-27.020331256158105
pbc="T T T"

Looks mostly ok otherwise

yes on it to fix it but is more delicate than initially thought... by time you are in london shall be done.

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.

Actually, we might want to write the info from the results dictionary, rather than via the calculate functions, so we clean the NaNs etc.

@ElliottKasoar
Copy link
Member

It looks like you've already done the equivalent changes for the Python tests, but test_singlepoint_cli.py::test_singlepoint fails because now that we don't write the results of the calculator directly, it no longer attaches a SinglePointCalculator when the data is read back in, and so get_potential_energy etc. can't be called.

Is this definitely what we want?

Might it be preferable to have some sort of store-as-info setting, rather than the default? Otherwise, it feels hidden away a little.

It also means if we do something like single_point.run() etc. repeatedly, it'll have to explicitly recalculate everything, when it might already be stored, just not where it expects.

@ElliottKasoar ElliottKasoar mentioned this pull request Jun 4, 2024
@alinelena alinelena force-pushed the atoms_info branch 2 times, most recently from 02f9a01 to ab150fe Compare June 5, 2024 05:36
@alinelena
Copy link
Member Author

just to note, this is an abuse of ase atoms object. till a proper mechanism to store user properties will be available

@alinelena alinelena force-pushed the atoms_info branch 2 times, most recently from 6a09fcf to 92233c2 Compare June 5, 2024 06:06
oerc0122
oerc0122 previously approved these changes Jun 5, 2024
Copy link
Collaborator

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

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

Apart from one stylistic thing, if this works, I think it's fine.

janus_core/calculations/single_point.py Outdated Show resolved Hide resolved
janus_core/calculations/single_point.py Outdated Show resolved Hide resolved
janus_core/calculations/single_point.py Outdated Show resolved Hide resolved
Co-authored-by: Jacob Wilkins <46597752+oerc0122@users.noreply.github.com>
Co-authored-by: Jacob Wilkins <46597752+oerc0122@users.noreply.github.com>
oerc0122
oerc0122 previously approved these changes Jun 5, 2024
Co-authored-by: Jacob Wilkins <46597752+oerc0122@users.noreply.github.com>
@alinelena alinelena merged commit 2b43729 into stfc:main Jun 5, 2024
8 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