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

Support new detector at SixS and new alias dict #279

Merged
merged 44 commits into from
Jul 23, 2022
Merged

Conversation

DSimonne
Copy link
Collaborator

@DSimonne DSimonne commented Jul 12, 2022

I created a new detector for SixS, added a new alias dict and fixed a pb in the rocking curve function.

This was tested locally but before the big id 27 changes

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update
  • Documentation update

How has this been tested?

Multiple scan processed during beamtime with gwaihir

Test Configuration:

Debian 11

Checklist:

  • I have made corresponding changes to the documentation
  • I have added corresponding unit tests
  • I have run doit and all tasks have passed

@DSimonne DSimonne requested a review from carnisj July 12, 2022 20:49
@clatlan
Copy link
Collaborator

clatlan commented Jul 13, 2022

I also wonder about the horizontal configuration.

Now at SixS we mostly use the vertical configuration, so I flip the data before preprocessing as you had adviced before (the detail is in gwaihir.controller.control_startup.rotate_sixs_data()).

But the rocking curve is perfomed outofplane for most of the scans;

  • we are using the 111 reflection,
  • the sample is vertical,
  • the inplane rocking angle is omega,
  • to move the detector inplane is delta and outofplane gamma.
  • we perform the rocking curve with mu, also outofplane (basically mu and gamma are in a theta - two theta geometry perpendicular to the sample plane)

However bcdi does not support outofplane for the rocking_angle parameter.

And in strain.py, the value of omega and gamma are switched (we are at mu ~ 19 deg and gamma ~ 38 deg)

INFO:bcdi.preprocessing.process_scan:Detector angles before correction: inplane 37.44 deg, outofplane 0.17 deg
INFO:bcdi.preprocessing.process_scan:Corrected detector angles: inplane 37.77 deg, outofplane 0.32 deg
INFO:bcdi.preprocessing.process_scan:Wavevector transfer of Bragg peak: [-0.90276104  0.0242599   2.63858146], Qnorm=2.7888

I'm not sure everything is right ?

I can put the data on the esrf if you would like to test ?

@carnisj
Copy link
Owner

carnisj commented Jul 13, 2022

Hi David.

Please request the review when all pipelines are passing. I have no time to debug it myself.

Regarding the geometry, I'm pretty sure that we already discussed about it. I'm against flipping the data. The package is built around two absolute reference frames (laboratory frame z downstream, y vertical up and x outboard) and the 'usual' reciprocal space frame (qx downstream, qz vertical up, qy outboard). The sample orientation is not relevant when deciding whether this is an inplane or outofplane rocking curve. Only the rocking motor matters. So if the rocking motor is phi (rotation axis around the vertical axis) then this is an inplane rocking curve, and you should setup your config file as such. If for a specific experiment, the sample substrate was vertical, then the sample will be correctly reconstructed in its absolute orientation (i.e. it will be vertical as well). You can post-process this if you wish to use another frame, but at least there is no ambiguity about its orientation at the output of the bcdi scripts.

@DSimonne
Copy link
Collaborator Author

Hi,

Oh ok I thought you had said it was best to rotate the diffraction pattern prior to the analysis, otherwise I'm not sure why I would have included that, I will try both approach and see the differences.

Thank you!

@DSimonne
Copy link
Collaborator Author

What I introduced

  • new detector (MerlinSixS)
  • changed the way the COM is found in bcdi_utils
  • create a mask when showing the rocking curve to skip duplicate values in the tilt angles
  • fix binning issue when only in the direction of the rocking curve (the data was unbinned also in the rocking curve direction without having binned before), I also changed the test

bcdi_utils.py, line 1155

    ####################################################################################
    # bin data and mask in the detector plane if not already done during loading       #
    # binning in the stacking dimension is done at the very end of the data processing #
    ####################################################################################

So maybe we need to move the binning in the rocking curve before so that the tests make more sense ?
Perhaps I missed smtg ...

@DSimonne DSimonne marked this pull request as ready for review July 18, 2022 09:18
@carnisj
Copy link
Owner

carnisj commented Jul 21, 2022

Ok in principle for adding the detector and the new alias_dict. Please remove the part about the method find_bragg and open an issue if you think there is one. This is not within the scope of this Pull Request and is also conflicting with changes applied on this function recently. We can have a look at this separately. Please also rebase onto the main branch.

tests/preprocessing/test_bcdi_utils.py Outdated Show resolved Hide resolved
@DSimonne
Copy link
Collaborator Author

Hi,
I think I rebased it ? But now it shows more changes which is weird bc these changes are already in your main branch right?
I'm not sure if it's right =D

@DSimonne DSimonne requested a review from carnisj July 22, 2022 13:57
@carnisj carnisj merged commit 5c241d7 into carnisj:main Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants