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

Less information is needed from the user when there is access to the raw data files #36

Open
jamesrhester opened this issue Feb 2, 2023 · 4 comments
Labels
enhancement New feature or request Tools Bugs in tools

Comments

@jamesrhester
Copy link
Contributor

The original issue_to_imgcif tool did not access the raw data. A tool with access to the raw data can:

  1. Suggest default axis names and stacking based on typical items found in known headers
  2. Ignore axes that do not move during any of the scans
  3. Determine the array dimensions from the CBF Mime header (guaranteed to be available)
@jamesrhester jamesrhester added enhancement New feature or request Tools Bugs in tools labels Feb 2, 2023
@julianhoersch
Copy link
Contributor

1.I suggest that I should get the axis names from the mini header information as it is currently done for the scan info and use those for the axis description as well (maybe ask the user if he want's to keep that?). For the axis stacking I'm not sure how to get that - should this be only suggested from the names of the axes? e.g. omega is the base
2. I need to include _prune_scan_info for this again - should this also remove those axes from the axis description or only the scan info?
3. will be added in julianhoersch@39a1175

@jamesrhester
Copy link
Contributor Author

  1. Yes, the axis stacking should just be based on typical axis names and stackings, and the user given the option to correct it. So from top to bottom that's phi/kappa or chi/omega. I noticed that some minicbf headers give an axis name but then a value of -9999 - I guess that means they don't exist.

  2. We should not include any axes in the imgCIF axis list or scan list for which we can't derive geometry information, i.e. axes that are always at their zero positions. If the database has an entry for the instrument then we can include the extra axes as that implies we do know the full geometry.

  3. A topic branch + pull request would work better I think.

@julianhoersch
Copy link
Contributor

How should the detector axes in this context be handled? Right now everything for the detector axes is hard coded:

# Insert assumed axis names and orientations
axis_id = ["source", "gravity", "two_theta", "trans", "detx", "dety"]
axis_type = ['.', '.', "rotation", "translation", "translation", "translation"]
equip = ["source", "gravity", "detector", "detector", "detector", "detector"]
depends_on = ['.', '.', '.', "two_theta", "trans", "detx"]

From the scan extractor of the mini cbf header from cbf_cyclohexane_crystal2 I obtain for example the axes ['chi', 'phi', 'detector_2theta', 'omega', 'distance']. I identify axes named ("phi", "kappa", "chi", "omega", "angle", "two_theta") as goniometer axes and axes named ("detector_2theta", "detector_distance", "dx", "trans", "distance") as detector axes.

# Configuration information
#TODO always?
ROT_AXES = ("chi", "phi", "detector_2theta", "two_theta", "omega",
"angle", "start_angle", "kappa")
TRANS_AXES = ("detector_distance", "dx", "trans", "distance")
ALWAYS_AXES = ("distance", "two_theta", "detector_2theta")

This identifies ("detector_2theta", "distance") as detector axes. Should irrespective of those axes the names be mapped to the hard coded ones or those names found in the scan be used?

hence:
every axis identified as translation ("detector_distance", "dx", "trans", "distance") should be replaced by trans
every axis identified as rotation (right now this is pointless if detector_2theta is the only one) mapped to detector_2theta

Is the distinction between two_theta for the goniometer and detector_2theta for the detector correct and they are not the same?

Can there be other names of detector axes? Should there be some possibility to define more / other axes from the user?

@jamesrhester
Copy link
Contributor Author

OK, so there are two options: (1) use our predefined list of axis names for the imgCIF, and ignore the names used in the minicbf headers; or (2) do our best to make the names in our imgCIF file match the names found in the miniCBF header.
Either option will produce a perfectly valid imgCIF file.

I suggest that if it is not too difficult, option (2) will lead to the behaviour that a user would expect and would make it easier to understand how the imgCIF file relates to the minicbf. Best would be to make it a command-line option (--preserve-axis-names) and make everybody happy, as long as that is not too tedious as a programmer. I don't think this is a priority but we should do it eventually.

every axis identified as translation ("detector_distance", "dx", "trans", "distance") should be replaced by trans
every axis identified as rotation (right now this is pointless if detector_2theta is the only one) mapped to detector_2theta

Note that the TRANS_AXES array simply lists all axis names that we have found that are translation axes, they could still be distinct axes, e.g. in the Bruker there are several translation axes for positioning the detector in X and Y. So they shouldn't all be replaced by a single name. Same goes for ROT_AXES

Is the distinction between two_theta for the goniometer and detector_2theta for the detector correct and they are not the same?

They are different names for the same rotation axis, and it is not a goniometer axis.

Can there be other names of detector axes? Should there be some possibility to define more / other axes from the user?

Yes, there can be many translation and rotation axes attached to the detector in order to position it in three dimensions. I suggest we don't worry about them until we get a file that actually has them in. The ones that I know of (Diamond, Bruker) emit an imgCIF file directly which is why we haven't needed to include them here. I think the ability to define an arbitrary axis should be provided eventually in the command line, but I don't think it is a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Tools Bugs in tools
Projects
None yet
Development

No branches or pull requests

2 participants