Open RichardWaiteSTFC opened 9 months ago
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:
PeaksWorkspace
so we can keep track of which peak was indexed (assigned HKL) with which UBIndexPeaks
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)Just some quick thoughts:
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).
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:
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
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?
@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...
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 byOrientedLattice::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 ifIndexPeaks
andPredictPeaks
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