Refactor Abins, replace SData with Euphonic data structures #37350

ajjackson · 2024-05-13T13:57:50Z

Description of work

Summary of work

Remove abins.sdata.SData, use classes from euphonic.spectra instead

Purpose of work

Abins currently manages collections of simulated spectra with an "SData" class. This assumes that the data will be structured in a tree with heirarchy atom -> quantum-order. This is not flexible enough to deal with coherent scattering, which might not be decomposable by atom.

Euphonic is an existing dependency and uses Spectrum1D, Spectrum2D and Spectrum1DCollection classes to manage spectra including arbitrary metadata. Effectively this is a flat table structure, amenable to "query" or "pipeline" workflows. This seems to be an appropriate tool, so here we extend that to add the missing Spectrum2DCollection and work towards using those natively in Abins.

Further detail of work

Add a method to SData to produce an equivalent SpectrumNDCollection object
Update abins to use these objects for all "output" management (i.e. summing relevant subsets and creating workspaces)
Update abins to simulate data directly into a SpectrumNDCollection object
Delete SData class and related unit tests
Move patches and extensions to Euphonic classes upstream into a new euphonic release
Remove Euphonic subclasses from mantid, update Euphonic dependency version

To test:

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

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?
Do the release notes conform to the release notes guide?

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.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

ajjackson · 2024-06-17T14:34:44Z

This initially led to some severe performance regressions, due to the relatively high overhead of iterating over a Euphonic Spectrum1DCollection/Spectrum2DCollection object, which performs unit conversions while spawning new objects. Some efficiency tweaks can be made to the Euphonic objects, but mostly we can recover the performance by iterating in other ways (over the metadata and underlying arrays).

In addition, we can start to introduce multiprocessing-based parallelism, which plays nicely with loops over "flat" tables.

ajjackson · 2024-06-21T15:37:06Z

Very strange test failure there: something is going wrong with the Pychop call inside a multiprocessing starmap, but only for OSX and Windows. I wonder if there is some OS-dependent namespace hackery that runs into issues because both abins and Pychop have an Instrument class 🤔

sf1919 · 2024-08-21T13:52:57Z

Apologies... wrong PR

github-actions · 2024-09-04T16:01:14Z

👋 Hi, @ajjackson,

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

sf1919 · 2025-01-16T16:19:20Z

@ajjackson is this PR still one to be worked on?

ajjackson · 2025-01-16T16:37:10Z

Yes it is still in progress. Effort moved upstream to Euphonic for a bit and will be back here shortly (with many deleted lines of code.)

ajjackson · 2025-01-28T16:15:08Z

Oof, that was a big rebase

github-actions · 2025-02-20T17:14:31Z

👋 Hi, @ajjackson,

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

ajjackson · 2025-03-12T11:35:52Z

Hurrah, we now have a substantial net reduction in lines-of-code 🎉

github-actions · 2025-04-11T12:45:10Z

👋 Hi, @ajjackson,

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

ajjackson · 2025-05-14T16:38:10Z

I have done a few large benchmark calculations now:

for this paper we ran Abins on supercells of ~1500 atoms https://arxiv.org/abs/2504.19352 ; this branch and main each took around 90 mins for calculations
in a separate benchmark using some NaCl force constants from the Euphonic training set (i.e. a small unit cell) with a large q-space sampling of ~97,000 q-points, both branches took 6.4 min to run

So I am confident that there is not a significant negative performance impact. (There is certainly scope to improve the performance in those cases by better use of parallelism, though!)

sf1919 · 2025-05-19T09:31:22Z

I think this needs to be rebased after the Python 3.11 upgrade because it's trying to use Python 3.10 looking at the console logs.

adriazalvarez

Thanks for the changes.

ajjackson · 2025-06-10T08:27:08Z

Thanks for review, hope it wasn't too painful relative to size!

cailafinn

Looking very good, just have a couple of questions before merge.

cailafinn · 2025-06-16T14:28:38Z

scripts/abins/abinsalgorithm.py

-        Helper function for calculating S for the given atomic index
+    def get_s(self, spectrum: Spectrum) -> Quantity:
+        """Get spectral quantity array, from y or z axis of Spectrum as appropriate"""
+        from abins.constants import ONE_DIMENSIONAL_INSTRUMENTS


Is there a reason for this only being imported at this point, rather than at the top of the file? Especially given some of the abins constants are already imported. There's a few of these throughout the changes, so just want to confirm.

No technical reason, just subjective tidiness which may have changed between iterations of refactoring! I could move it to the top if you like.

That's the standard for almost all the files in mantid, so that would be preferable. It just means any import errors get caught as early as possible

Ok, I have moved all the imports in abinsalgorithm.py to top-level.

In the process I noticed the sanity-check of cpu_count() seemed to be a bit broken and cleaned it up.

scripts/test/Abins/AbinsSDataTest.py

- Use stdlib multiprocessing, which is actually used by the code - import at top level - accept None gracefully (the default and recommended setting)

cailafinn

Great, looks good to me now. Just wanted to check if this has any user-facing behaviour changes? If so, could we have a release note?

ajjackson · 2025-06-17T14:21:50Z

It's not supposed to, unless they are importing things from the scripts directory and fiddling with advanced parameters. The performance profile will have slightly changed but I benchmarked this and it doesn't seem to be noticeable at this point.

Perhaps it is worth writing that there was a big refactor of the internal library for AbINS, as it could in theory impact users who do clever things with external scripts that hook into AbINS. Most likely, I am the only one of those 😅

Some small bits of refactoring pulled out of mantidproject#37350

…oject#37350) ### Description of work #### Summary of work Remove `abins.sdata.SData`, use classes from `euphonic.spectra` instead #### Purpose of work Abins currently manages collections of simulated spectra with an "SData" class. This assumes that the data will be structured in a tree with heirarchy atom -> quantum-order. This is not flexible enough to deal with coherent scattering, which might not be decomposable by atom. Euphonic is an existing dependency and uses Spectrum1D, Spectrum2D and Spectrum1DCollection classes to manage spectra including arbitrary metadata. Effectively this is a flat table structure, amenable to "query" or "pipeline" workflows. This seems to be an appropriate tool, so here we extend that to add the missing Spectrum2DCollection and work towards using those natively in Abins. #### Further detail of work - [x] Add a method to SData to produce an equivalent SpectrumNDCollection object - [x] Update abins to use these objects for all "output" management (i.e. summing relevant subsets and creating workspaces) - [x] Update abins to simulate data directly into a SpectrumNDCollection object - [x] Delete SData class and related unit tests - [x] Move patches and extensions to Euphonic classes upstream into a new euphonic release - [x] Remove Euphonic subclasses from mantid, update Euphonic dependency version

Some small bits of refactoring pulled out of mantidproject#37350

…oject#37350) ### Description of work #### Summary of work Remove `abins.sdata.SData`, use classes from `euphonic.spectra` instead #### Purpose of work Abins currently manages collections of simulated spectra with an "SData" class. This assumes that the data will be structured in a tree with heirarchy atom -> quantum-order. This is not flexible enough to deal with coherent scattering, which might not be decomposable by atom. Euphonic is an existing dependency and uses Spectrum1D, Spectrum2D and Spectrum1DCollection classes to manage spectra including arbitrary metadata. Effectively this is a flat table structure, amenable to "query" or "pipeline" workflows. This seems to be an appropriate tool, so here we extend that to add the missing Spectrum2DCollection and work towards using those natively in Abins. #### Further detail of work - [x] Add a method to SData to produce an equivalent SpectrumNDCollection object - [x] Update abins to use these objects for all "output" management (i.e. summing relevant subsets and creating workspaces) - [x] Update abins to simulate data directly into a SpectrumNDCollection object - [x] Delete SData class and related unit tests - [x] Move patches and extensions to Euphonic classes upstream into a new euphonic release - [x] Remove Euphonic subclasses from mantid, update Euphonic dependency version

This pulls a variety of changes to buildscripts, dependencies, and assundries upstream of those changes. * #39108 * #39440 * #38976 * #39240 * #39522 * #39337 * #39481 * #39295 * #39360 * #39355 * #39170 * #39343 * #37350 --------- Co-authored-by: Darsh Dinger <106342815+darshdinger@users.noreply.github.com> Co-authored-by: Jack <jack.allen@stfc.ac.uk> Co-authored-by: thomashampson <thomas.hampson@stfc.ac.uk> Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com> Co-authored-by: Sarah Foxley <55837273+sf1919@users.noreply.github.com> Co-authored-by: Adam J. Jackson <a.j.jackson@physics.org>

…37350)" This reverts commit 5894d04.

…37350" (#39665) This reverts commit 5894d04.

…Euphonic features (#39669) ### Description of work This reinstates #37350 , which was reverted in #39665 This was done in order to stop breakage of package build tests on non-Linux platforms. It seems this is caused by an incompatibility between multiprocessing "spawn" and the systemtest code, which also spawns Python subprocesses. This mechanism is not supposed to be nested. On Linux the default mechanism is "fork" which seems to work in parallel systemtest, but from Python 3.14 this default will also change to "spawn" and start failing. #### Summary of work - The branch from #37350 has been rebased back onto `main` - The conditions for hanging test suites were investigated - Multiprocessing code is removed from Abins and replaced by serial `map` operations. - The performance impact seems to be small, due to optimisations in Euphonic which came after the parallel operations were originally introduced to the branch. #### Purpose of work See #37350 #### Further detail of work It's not great that multiprocessing is incompatible with the systemtest suite. In this case we can manage without, but there is a good chance we will want to use it in future. Multithreading is probably ok, but these are not equivalent tools. ### To test: Everything is covered by CI. In addition to the default stuff we need to check `build_packages_from_branch` on non-Linux platforms as this was breaking. This shouldn't require a release note because #37350 didn't. --- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

…antidproject#37350" (mantidproject#39665) This reverts commit 5894d04.

…Euphonic features (mantidproject#39669) ### Description of work This reinstates mantidproject#37350 , which was reverted in mantidproject#39665 This was done in order to stop breakage of package build tests on non-Linux platforms. It seems this is caused by an incompatibility between multiprocessing "spawn" and the systemtest code, which also spawns Python subprocesses. This mechanism is not supposed to be nested. On Linux the default mechanism is "fork" which seems to work in parallel systemtest, but from Python 3.14 this default will also change to "spawn" and start failing. #### Summary of work - The branch from mantidproject#37350 has been rebased back onto `main` - The conditions for hanging test suites were investigated - Multiprocessing code is removed from Abins and replaced by serial `map` operations. - The performance impact seems to be small, due to optimisations in Euphonic which came after the parallel operations were originally introduced to the branch. #### Purpose of work See mantidproject#37350 #### Further detail of work It's not great that multiprocessing is incompatible with the systemtest suite. In this case we can manage without, but there is a good chance we will want to use it in future. Multithreading is probably ok, but these are not equivalent tools. ### To test: Everything is covered by CI. In addition to the default stuff we need to check `build_packages_from_branch` on non-Linux platforms as this was breaking. This shouldn't require a release note because mantidproject#37350 didn't. --- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

ajjackson force-pushed the sdata-euphonic branch from 7dafc5f to 4130746 Compare May 16, 2024 08:45

ajjackson force-pushed the sdata-euphonic branch from c2e4ae9 to fcd1115 Compare May 24, 2024 16:49

ajjackson force-pushed the sdata-euphonic branch from 835cf29 to 7bcf9d4 Compare August 6, 2024 16:07

sf1919 added Has Conflicts Used by the bot to label pull requests that have conflicts and removed Has Conflicts Used by the bot to label pull requests that have conflicts labels Aug 21, 2024

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Sep 4, 2024

ajjackson force-pushed the sdata-euphonic branch from 05536ab to 75f77f1 Compare January 28, 2025 16:14

github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 28, 2025

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 20, 2025

ajjackson force-pushed the sdata-euphonic branch from fd4be0d to 02e7887 Compare February 21, 2025 13:28

github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 21, 2025

ajjackson force-pushed the sdata-euphonic branch from 1f87fb2 to 6c119a5 Compare March 10, 2025 17:25

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Apr 11, 2025

ajjackson force-pushed the sdata-euphonic branch from 15433c5 to c4a67f2 Compare April 15, 2025 12:24

github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Apr 15, 2025

ajjackson force-pushed the sdata-euphonic branch from c0efba8 to 889fd26 Compare May 19, 2025 08:48

ajjackson force-pushed the sdata-euphonic branch from 5251436 to cd9528f Compare May 20, 2025 08:27

ajjackson marked this pull request as ready for review May 20, 2025 08:28

adriazalvarez previously approved these changes Jun 10, 2025

View reviewed changes

github-project-automation bot moved this from Waiting response to Approved in DEVS Jun 10, 2025

cailafinn self-assigned this Jun 16, 2025

cailafinn requested changes Jun 16, 2025

View reviewed changes

github-project-automation bot moved this from Approved to Waiting response in DEVS Jun 16, 2025

Move abins.constants imports to top of file

41033d3

ajjackson dismissed adriazalvarez’s stale review via 41033d3 June 17, 2025 09:05

ajjackson added 2 commits June 17, 2025 09:57

Simplify abins.parameters.performance["threads"] check

656261d

- Use stdlib multiprocessing, which is actually used by the code - import at top level - accept None gracefully (the default and recommended setting)

Move another import to top-level

f534861

cailafinn reviewed Jun 17, 2025

View reviewed changes

cailafinn approved these changes Jun 17, 2025

View reviewed changes

github-project-automation bot moved this from Waiting response to Approved in DEVS Jun 17, 2025

cailafinn merged commit 5894d04 into mantidproject:main Jun 17, 2025
10 checks passed

github-project-automation bot moved this from Approved to Merged in DEVS Jun 17, 2025

github-project-automation bot moved this from Ready for Review to Done in Mantid Maintenance Jun 17, 2025

peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this pull request Jun 20, 2025

Abins cleanup for 37350 (mantidproject#39343)

8d8df50

Some small bits of refactoring pulled out of mantidproject#37350

peterfpeterson mentioned this pull request Jun 20, 2025

Dependencies and buildscripts from main #39531

Merged

peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this pull request Jun 20, 2025

Abins cleanup for 37350 (mantidproject#39343)

9147543

Some small bits of refactoring pulled out of mantidproject#37350

sf1919 added a commit that referenced this pull request Jul 14, 2025

Revert "Refactor Abins, replace SData with Euphonic data structures (#…

45a3427

…37350)" This reverts commit 5894d04.

MohamedAlmaki added a commit that referenced this pull request Jul 14, 2025

Revert "Refactor Abins, replace SData with Euphonic data structures (#…

0251a96

…37350)" This reverts commit 5894d04.

peterfpeterson pushed a commit that referenced this pull request Jul 14, 2025

Revert "Refactor Abins, replace SData with Euphonic data structures #…

32f01be

…37350" (#39665) This reverts commit 5894d04.

ajjackson mentioned this pull request Jul 21, 2025

Restore reverted change: Abins refactor replacing SData classes with Euphonic features #39669

Merged

peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this pull request Sep 24, 2025

Revert "Refactor Abins, replace SData with Euphonic data structures m…

4569af4

…antidproject#37350" (mantidproject#39665) This reverts commit 5894d04.

Refactor Abins, replace SData with Euphonic data structures #37350

Refactor Abins, replace SData with Euphonic data structures #37350

Uh oh!

Conversation

ajjackson commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of work

Summary of work

Purpose of work

Further detail of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

Uh oh!

ajjackson commented Jun 17, 2024

Uh oh!

ajjackson commented Jun 21, 2024

Uh oh!

sf1919 commented Aug 21, 2024

Uh oh!

github-actions bot commented Sep 4, 2024

Uh oh!

sf1919 commented Jan 16, 2025

Uh oh!

ajjackson commented Jan 16, 2025

Uh oh!

ajjackson commented Jan 28, 2025

Uh oh!

github-actions bot commented Feb 20, 2025

Uh oh!

ajjackson commented Mar 12, 2025

Uh oh!

github-actions bot commented Apr 11, 2025

Uh oh!

ajjackson commented May 14, 2025

Uh oh!

sf1919 commented May 19, 2025

Uh oh!

adriazalvarez left a comment

Choose a reason for hiding this comment

Uh oh!

ajjackson commented Jun 10, 2025

Uh oh!

cailafinn left a comment

Choose a reason for hiding this comment

Uh oh!

cailafinn Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

ajjackson Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

cailafinn Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

ajjackson Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cailafinn left a comment

Choose a reason for hiding this comment

Uh oh!

ajjackson commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

ajjackson commented May 13, 2024 •

edited

Loading