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

Abins: import force constants data using Euphonic #32220

Merged
merged 19 commits into from
Nov 30, 2021

Conversation

ajjackson
Copy link
Contributor

@ajjackson ajjackson commented Aug 17, 2021

Description of work.

Builds on (and no longer blocked by) #30760

Use the new semi-optional Euphonic dependency to allow import from force-constant data in .castep_bin (CASTEP), .yaml (Phonopy) or .json (Euphonic native) formats.

This greatly expands the range of atomistic codes that could be used with Abins (via Phonopy) and lays the groundwork for implementing more sophisticated simulation schemes that properly account for energy-q relationships.

To test:
Manual testing

See #30760 testing guide for some suitable input files.

Open Abins Algorithm, select ForceConstants as input file type and run with .castep_bin, .yaml etc. files.
(For Euphonic .json try something from the Euphonic tests_and_analysis/test/data/force_constants directory.)
If Euphonic is not installed, the VibrationalOrPhononFile field should fail to validate, with a tooltip advising to install Euphonic with an installer from the ScriptRepository. (This should not be necessary for Conda-built Mantid.)

The q-point sampling mesh is determined by a variable in abins.parameters. This setup is used somewhat consistently in Abins to allow "advanced" options to be tweaked. The default value is on the high side to be safe, but the best-practice would be to check convergence. (This is not an option for other formats which use pre-computed phonon modes.) If Abins is re-run after changing abins.parameters.sampling['force_constants']['qpt_cutoff'], the Abins log should show that this invalidated the cache, and a new set of scattering spectra is calculated. The Monkhorst-Pack grid (i.e. regular divisions of the reciprocal unit cell) is related to the cutoff; the sampling is reported in the log and each direction should scale linearly with the cutoff and inversely with the crystalline unit cell size. (Values are rounded up so this behaviour may be less clear for large cells.)

Unit testing
At the moment there is no automatic unit test for this feature. This is because Euphonic is not yet available on the unit-testing CI. I propose to revisit this when the CI is running on a Conda build. But perhaps it would be an idea to include some kind of skippable test before then, which runs if Euphonic is available? It would be easy enough to hack something, but do we have a standard/maintainable way of doing that?

There is no associated issue.


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

@AnthonyLim23 AnthonyLim23 added the ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS label Aug 18, 2021
@AnthonyLim23 AnthonyLim23 added this to the Release 6.3 milestone Aug 18, 2021
@ajjackson
Copy link
Contributor Author

This is going very well: in this plot I was able to reproduce almost exactly the same curve by importing the .phonon file and .castep_bin file from the same phonon calculation. In the latter case, the data was "interpolated" to the same 2x2x2 mesh as the exact Dynamical Matrix points in the .phonon file. The .phonon file import uses the existing Abins importer, while the .castep_bin file contains force constants and was imported with the new Euphonic infrastructure.

Finally I included a line computed with more interpolated q-points: 7x7x7 grid. The difference is quite dramatic and illustrates just how sensitive this method is to q-point sampling when small unit cells are involved. Evidently these systems should be treated with care: Abins sums over all q-points, assuming the energy-q relationship is weak for a powder-averaged measurement. That assumption is instrument-dependent and likely to break down in such cases.

castep-bin-vs-phonon-file

@github-actions
Copy link

👋 Hi, @ajjackson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@ajjackson ajjackson force-pushed the abins-euphonic branch 2 times, most recently from 0f0f98a to e5d88ba Compare September 22, 2021 15:41
@github-actions github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Sep 23, 2021
@github-actions
Copy link

👋 Hi, @ajjackson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Sep 24, 2021
@github-actions github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Sep 24, 2021
@ajjackson ajjackson force-pushed the abins-euphonic branch 3 times, most recently from 749d340 to 8713e78 Compare September 30, 2021 13:34
@github-actions github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Oct 4, 2021
@github-actions
Copy link

github-actions bot commented Oct 4, 2021

👋 Hi, @ajjackson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Oct 4, 2021
Use Euphonic library to sample force constants as part of Abins. This
makes it possible to use Abins with Phonopy.

Some more details need to be worked out:
- provide a way to set sampling cutoff (advanced parameter?)
- validate against other methods to ensure eigenvectors are scaled
  correctly. (CASTEP would be simplest option as we can compare
  .phonon and .castep_bin outputs.)
- Input files are hashed in order to identify if they have
  changed (and invalidate cached results).
- The existing implementation assumed that these files are always
  text, and decodes to utf-8 before hashing.
- With support for .castep_bin files via Euphonic, that assumption is
  no longer safe. To support this, we drop the text decoding and work
  with binary directly.
- It's not clear that decoding the text provided any benefit, so the
  binary approach could be used for all input files (avoiding any need
  for extra logic.)
- This change will cause existing hashes (and hence Abins run caches)
  not to be recognised. That's not a big deal, there have been plenty
  of other compatibility breaks between Abins versions and old caches.
Somehow this escaped the last couple of commits!
Abins hashes input files for cache invalidation purposes. The hashing
target has changed from text to binary for compatibility with more
file types. As expected, this breaks tests that compare Abins'
input file hash with reference data. Here two of those test data files
are updated with the new hashes. (The input files have not changed,
only the hashing method.)
- see explanation in previous commit (GAUSSIAN data)
- note that not all the cases needed updating; presumably it is
  possible for text and bytes hash to agree depending on encoding?
See previous commits
This unit test will be skipped unless Euphonic is available, so for
now will not run on the CI. But at least we can check locally, and it
_should_ start working when CI is moved to a Conda build.
SingleCrystal was removed a while ago!
Systemtest was broken by introducing acoustic sum rule correction as
default behaviour. For consistency with previous behaviour,
SimulatedDensityOfStates now explicitly suppresses ASR.

Would it be better if DOS used ASR? Maybe, but that can be decided in
another set of changes. For now let's avoid the "regression".
@ajjackson ajjackson marked this pull request as ready for review October 14, 2021 09:21
@AnthonyLim23 AnthonyLim23 self-assigned this Oct 26, 2021
scripts/abins/io.py Outdated Show resolved Hide resolved
@AnthonyLim23
Copy link
Contributor

Could you please send me some example data?

Co-authored-by: Anthony <anthony.lim@stfc.ac.uk>
@ajjackson
Copy link
Contributor Author

Could you please send me some example data?

Some sample data can be found in the Euphonic test suite: I suggest the quartz-666-grid.phonon and quartz.castep_bin files from https://github.com/pace-neutrons/Euphonic/tree/master/tests_and_analysis/test/data/castep_files/quartz and mp-7041-20180417.yaml file from https://github.com/pace-neutrons/Euphonic/tree/master/tests_and_analysis/test/data/phonopy_files/CaHgO2 . Also grab a CASTEP benzene example (i.e. to check existing functionality) from existing Mantid test data: Data/UnitTest/benzene_Abins.phonon (MD5 ec53aa8ff74b7cd257d7a82c3d6547fd).

@AnthonyLim23
Copy link
Contributor

using ForceConstants and the .phonon file crashes python

@ajjackson
Copy link
Contributor Author

ajjackson commented Nov 23, 2021

using ForceConstants and the .phonon file crashes python

Full-on crash, not just an Exception? Interesting! The .phonon file is supposed to be used with the CASTEP option, so this is an incompatible format. I guess the loader needs to be made a touch more robust against incompatible file input.

@AnthonyLim23
Copy link
Contributor

yes please. It was a full on crash of Python (not mantid)

@ajjackson
Copy link
Contributor Author

ajjackson commented Nov 23, 2021

Right, I see two things here.

  • Other options enforce a file extension. (e.g. if you pick a .phonon file and GAUSSIAN, the inputs fail validation and it doesn't try to run.) I'm not a big fan of enforcing file extensions, but as it would be consistent with the existing behaviour and make it harder to do this accidentally, we could add it.

  • The traceback I get is

********* UNHANDLED EXCEPTION *********
  what(): 'ValueError' object has no attribute 'message'
  at line 82 in '/mantid_src/Framework/PythonInterface/plugins/algorithms/Abins.py'

i.e. there's an error in the code which assumes Exceptions will have a .message attribute, but it turns out that ValueError doesn't. Easy to work around.

ValueError does not have a .message attribute, which was causing a
further error during the f-string evaluation. The general case is
covered by checking for a message attribute.
@ajjackson
Copy link
Contributor Author

Ok, now the ValueError is read correctly and leads to meaningful feedback in the Algorithm GUI 🎉

@AnthonyLim23
Copy link
Contributor

what option does the castep_bin file work with? I would expect castep, but it fails. I also cant get the yml file to work either.

@ajjackson
Copy link
Contributor Author

what option does the castep_bin file work with? I would expect castep, but it fails. I also cant get the yml file to work either.

.castep_bin should work with the ForceConstants option, as we are reading force-constants data. The CASTEP option is used for .phonon files (i.e. frequencies/displacements at pre-specified q-points).

I agree this is infuriating and it would be nice to ditch the list of codes as it doesn't make a lot of sense when some codes produce multiple formats and some import methods support multiple codes. I think that could be a separate bit of work though, as it will need some tedious/robust automatic determination of file type...

What happens when the yaml file doesn't work? You should get a nice tooltip from the red asterisk telling you to install Euphonic.

Copy link
Contributor

@AnthonyLim23 AnthonyLim23 left a comment

Choose a reason for hiding this comment

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

From what I can tell, this all works as expected :shipit:

@gemmaguest gemmaguest merged commit 3d43e38 into mantidproject:main Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants