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

Investigate supporting multiple UBs in matrix and peak workspace #36592

Open
RichardWaiteSTFC opened this issue Dec 23, 2023 · 4 comments · May be fixed by #37166
Open

Investigate supporting multiple UBs in matrix and peak workspace #36592

RichardWaiteSTFC opened this issue Dec 23, 2023 · 4 comments · May be fixed by #37166
Assignees
Labels
Diffraction Issues and pull requests related to diffraction Investigation A task to investigate options for future work ISIS Team: Diffraction Issue and pull requests managed by the Diffraction subteam at ISIS
Milestone

Comments

@RichardWaiteSTFC
Copy link
Contributor

RichardWaiteSTFC commented Dec 23, 2023

Describe the outcome that is desired.
This relates to the multiple UB EPIC
https://isisneutronmuon.atlassian.net/browse/SS-25?atlOrigin=eyJpIjoiODI0NDgxMTAzODZkNDk0OWI5ODJhZDZmM2E1ODZhZDMiLCJwIjoiaiJ9)

The aim of the project is to allow multiple related UBs (e.g. a set of UBs related by a given rotation) to be optimised at a time, and for peak integration methods to be able to tell when a peaks from different UBs overlap and have been integrated together.

Does the outcome relate directly to a problem? Please describe.

The aim is to have the OrientedLattice class contain a collection of UBs instead of a single UB and for PeaksWorkspaces to have an extra attribute which is the index of the UB used to index the peak (i.e. assign it miller indices HKL).
Backwards compatibility will have to be preserved so by default the first UB should be returned by OrientedLattice::getUB() and similar for any setters etc.

The following algorithms (at the very least) will then need to be updated to select the UB required, or loop over them:

  • IndexPeaks
  • PredictPeaks
  • PredictFractionalPeaks
  • PredictSattelitePeaks
  • FindUBUsingIndexedPeaks
  • SetUB

The PeaksWorkpace table view will also need updating to display the UB index.

The aim of this investigation is to make the changes to OreintedLattice and see if IndexPeaks and PredictPeaks can be made to work, and then see what breaks...It would also be nice to add the column to the table view.

Additional context
Details of UB matrices and indexing can be found here
https://docs.mantidproject.org/v6.1.0/concepts/Lattice.html

@RichardWaiteSTFC RichardWaiteSTFC added Diffraction Issues and pull requests related to diffraction ISIS Team: Diffraction Issue and pull requests managed by the Diffraction subteam at ISIS Investigation A task to investigate options for future work labels Dec 23, 2023
@RichardWaiteSTFC RichardWaiteSTFC added this to the Release 6.10 milestone Dec 23, 2023
@RichardWaiteSTFC
Copy link
Contributor Author

RichardWaiteSTFC commented Jan 18, 2024

Once we can show with proof-of-concept PR #36654 we can do this without breaking anything, and @zjmorgan and @AndreiSavici are happy in principle, there are some next steps:

Changes to algorithms that set UB / oriented lattice (to preserve default behaviour, this will have to overwrite the set UB if there is only one, optionally we can set others at a specific index - we need to think about this aspect)

  • SetUB
  • FindUBUing<InsertMethod> e.g. FindUBUsingLatticeParameters
  • CalculateUMatrix

Next step after that (once we can set multiple UBs) is to make use of them!
This will involve:

  • Changes to PeaksWorkspace so we can keep track of which peak was indexed (assigned HKL) with which UB
  • Algorithms that use the UB to index (e.g. IndexPeaks and PredictPEaks, PredictFractionalPeaks) which will need to accept a UB index or loop over UB indices (again to preserve behaviour, all these will by default use the first UB defined)

@AndreiSavici
Copy link
Member

AndreiSavici commented Jan 18, 2024

Just some quick thoughts:

  • multiple UBs do not make sense to be stored on a single workspace. Suppose I have 2 UB matrices with a Matrix Workspace, and I want to convert to multi-dimensional workspace in HKL. Which one do I choose? Maybe both? Add some weight?
  • each UB is assigned to a set of peaks. If separated correctly, each set of peaks has a unique UB. So for each set of peaks the current peaks workspace makes sense.
  • what seems to be missing, the multiple domains/twins, I think should be implemented as a group of peak workspaces. Then we should have algorithms to generate these groups and to refine the UB in a simultaneous way (keeping the B the same). Further, we should be able to make a UI to visualize the workspace group as a single table with an extra column, and to allow use of the workspace group in the visualization, similar to the way we use peak workspace, with automatic color labeling for each underlying peaks workspace.
  • With the proposed implementation, there is no change to underlying data structure, just changing some algorithms, user interfaces

@RichardWaiteSTFC
Copy link
Contributor Author

Thanks for getting back to us so quickly @AndreiSavici, some interesting ideas it would be good to discuss.
It's bad timing but I'm about to go on leave for a month - so apologies I thought I better dump a lot of ideas in this comment.
It would be good to have a call when I'm back, rest assured we weren't planning to get this in suddenly, there's time to change tack!

Just some quick thoughts:

  • multiple UBs do not make sense to be stored on a single workspace. Suppose I have 2 UB matrices with a Matrix Workspace, and I want to convert to multi-dimensional workspace in HKL. Which one do I choose? Maybe both? Add some weight?

My plan was to give the user the option of selecting a single UB to use when converting to HKL - in any case with twining or multiple xtals the HKL plots look rather messy so I don't imagine people who want to make use of multiple UBs will want to do this.

If they were to do this, it would be handy if one could index the cursor position in sliceviewer with the other UBs to identify spurious peaks, or even better overlay a peak from a different domain/UB and plot the positions in the HKL slices, but this is a minor thing.

  • each UB is assigned to a set of peaks. If separated correctly, each set of peaks has a unique UB. So for each set of peaks the current peaks workspace makes sense.
  • what seems to be missing, the multiple domains/twins, I think should be implemented as a group of peak workspaces. Then we should have algorithms to generate these groups and to refine the UB in a simultaneous way (keeping the B the same). Further, we should be able to make a UI to visualize the workspace group as a single table with an extra column, and to allow use of the workspace group in the visualization, similar to the way we use peak workspace, with automatic color labeling for each underlying peaks workspace.
  • With the proposed implementation, there is no change to underlying data structure, just changing some algorithms, user interfaces

That's a good idea I hadn't thought of - I like the proposal to change the table view rather than the existing data structures (namely PeaksWorkspace and Sample`), but I have some questions:

  • Most of the time a peak will be indexed by only one UB, but e.g. in the case of orthorhombic twinning there is a family of reflections along the two-fold rotation axis which would be indexed by both - we will need to think how to handle this (there are other examples).

    • If the peaks were in separate tables would we duplicate the peak in all of them?
    • How do we avoid such peaks being over-counted when e.g optimising UBs, calibrating detectors etc.?
  • Practically we index peaks within some tolerance, for slight twinning then it is not unusual to be able to index a peak within tolerance using both UBs from different domains. We start with a table of found peaks from all domains, we need to be able to determine which UB indexes the found peak the best (i.e. classify peaks as belonging to one domain or another), and this might change once we start tweaking/optimising the UBs. The drawbacks of multiple tables is then:

    • We would then need to move a peak from one table to another to change UB - maybe it makes sense to put them in a single table?
    • Each peak table would know nothing of the other related table, a subset could be replaced with peaks from an entirely different run quite easily.
    • Where would we store information about the tables that are related and the twinning matrices we might want to make use of - we would need a completely new data structure with a lot of boiler plate stuff .
  • Is it actually easier/safer to change the existing data structure (that we should have good test coverage for the existing behaviour) than add a new one (special group of peaks workspaces) and support it in many algorithms? With all the casting, if/else it could add to many more branches through the code - and not all algorithms have great unit test coverage).

  • A lot of algorithms would need to be run on all peaks together anyway, and adding another loop over tables might not play well with existing parallelisation

    • For example for detector calibration we want to be able to use all peaks found - once the data structure had been changed (e.g. each peak has pointer to a UB - as it does for the goniometer) then many algorithms would require only minor changes.
    • Another example is integration, we want to integrate all peaks together to check for overlaps (from different domains and the same domain if e.g. a large unit cell), so we would need to add additional loops over many tables if they were kept separate and that's a much bigger change.

Maybe the above are not a big deal, I know we shouldn't be changing such core data structures lightly, but as long as we're careful to preserve default behaviour by design then I think it could save us some time and effort. What do you think?

@RichardWaiteSTFC
Copy link
Contributor Author

@MohamedAlmaki - following discussions with @AndreiSavici and @zjmorgan (thanks both) the plan is to instead use group of peak workspaces (each of which a different UB) and write a new table view so users can view these in a single table. Most algorithms (e.g. IndexPeaks) when called on a group will execute on each individual workspace - so minimal changes required there. I think with this plan there will be new algorithms required to handle integration or UB optimisation with multiple domains present (i.e. we won't try to adapt the existing algorithms). We also require instrument view and sliceviewer to support overlaying a group of peak workspaces - this isn't currently done, but should be fairly easy...

@MohamedAlmaki MohamedAlmaki linked a pull request Apr 24, 2024 that will close this issue
@sf1919 sf1919 modified the milestones: Release 6.10, Release 6.11 May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Diffraction Issues and pull requests related to diffraction Investigation A task to investigate options for future work ISIS Team: Diffraction Issue and pull requests managed by the Diffraction subteam at ISIS
Projects
Status: No status
Status: In Review
Development

Successfully merging a pull request may close this issue.

4 participants