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

Add geometry optimisation #33

Merged
merged 14 commits into from
Feb 23, 2024
Merged

Add geometry optimisation #33

merged 14 commits into from
Feb 23, 2024

Conversation

ElliottKasoar
Copy link
Member

@ElliottKasoar ElliottKasoar commented Feb 16, 2024

Resolves #32

This will require rebasing once #31 is merged.

To do:

  • Check atoms returned correctly
  • Add functionality to return intermediate structures
  • Add functionality to save optimised structure

@ElliottKasoar ElliottKasoar added enhancement New/improved feature or request janus labels Feb 16, 2024
@ElliottKasoar ElliottKasoar self-assigned this Feb 16, 2024
@ElliottKasoar ElliottKasoar changed the title Add geo opt Add geometry optimisation Feb 16, 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.

Mostly stylistic, but some over-generalisations on type-hints which would be nice to address.

janus_core/geo_opt.py Outdated Show resolved Hide resolved
janus_core/geo_opt.py Outdated Show resolved Hide resolved
janus_core/geo_opt.py Outdated Show resolved Hide resolved
janus_core/geo_opt.py Outdated Show resolved Hide resolved
tests/test_geo_opt.py Outdated Show resolved Hide resolved
tests/test_geo_opt.py Outdated Show resolved Hide resolved
tests/test_geo_opt.py Outdated Show resolved Hide resolved
@ElliottKasoar ElliottKasoar force-pushed the add-geo-opt branch 2 times, most recently from 32a9962 to 56fffd8 Compare February 20, 2024 14:44
@ElliottKasoar
Copy link
Member Author

  • Add functionality to return intermediate structures

This was actually already possible via opt_kwargs (see example in unit tests), which I think is sufficient, rather than adding another argument.

As with single point calculations, we need to document this soon and add examples (#34), even more so now the tests are a little harder to read off.

@ElliottKasoar
Copy link
Member Author

Mostly stylistic, but some over-generalisations on type-hints which would be nice to address.

Thanks for the suggestions! I think I've addressed most of them, with a couple of slightly open questions/comments.

One thing you may notice is that there's now a mix of Union and x | y typing. When we include from __future__ import annotations, pyupgrade prefers x | y where possible.

However, this seemed to break linking to ASE's documentation via intersphinx_mapping, so I've reverted back to Union where necessary.

janus_core/geom_opt.py Outdated Show resolved Hide resolved
janus_core/single_point.py Outdated Show resolved Hide resolved
@ElliottKasoar ElliottKasoar marked this pull request as ready for review February 20, 2024 15:32
oerc0122
oerc0122 previously approved these changes Feb 20, 2024
janus_core/single_point.py Show resolved Hide resolved
@ElliottKasoar
Copy link
Member Author

ElliottKasoar commented Feb 20, 2024

One additional option I've realised that @alinelena included in lavello/md.py is to read the saved trajectory (a pickled file) and rewrite e.g. as an extxyz:

  x = read(traj, index=":")
  write(opt_traj, x, format="extxyz")
  try:
     p = pathlib.Path(traj)
     p.unlink()
  except:
     pass

Do we want to add this in as well, or is the current option of saving the .traj binary file enough?

@oerc0122
Copy link
Collaborator

oerc0122 commented Feb 20, 2024

One additional option I've realised that @alinelena included in lavello/md.py is to read the saved trajectory (a pickled file) and rewrite e.g. as an extxyz:

  x = read(traj, index=":")
  write(opt_traj, x, format="extxyz")
  try:
     p = pathlib.Path(traj)
     p.unlink()
  except:
     pass

Do we want to add this in as well, or is the current option of saving the .traj binary file enough?

Why is the Path creation in the try? Also, never use a bare except, unlink also has missing_ok=True for this situation.

@ElliottKasoar
Copy link
Member Author

Why is the Path creation in the try? Also, never use a bare except, unlink also has missing_ok=True for this situation.

I was mostly just quoting the code that was in md.py for the gist, but good points, I'll fix those if we decide it's worth adding.

conftest.py Show resolved Hide resolved
@ElliottKasoar
Copy link
Member Author

One additional option I've realised that @alinelena included in lavello/md.py is to read the saved trajectory (a pickled file) and rewrite e.g. as an extxyz:

The way this is done isn't quite a neat as I'd like, as you specify both the .traj file and the .xyz file, but I thought it's probably safer to require both be specified since the former is deleted after creation of the latter.

I also changed saving the optimised structure to act in the same way as I've implemented this - requiring filename in the kwargs rather than its own variable, so it's slightly more condensed.

janus_core/geom_opt.py Outdated Show resolved Hide resolved
janus_core/single_point.py Outdated Show resolved Hide resolved
janus_core/single_point.py Outdated Show resolved Hide resolved
janus_core/geom_opt.py Outdated Show resolved Hide resolved
oerc0122
oerc0122 previously approved these changes Feb 23, 2024
@alinelena alinelena merged commit ead43ec into stfc:main Feb 23, 2024
12 checks passed
@ElliottKasoar ElliottKasoar deleted the add-geo-opt branch February 23, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New/improved feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add geometry optimisation
3 participants