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

dials.export format=shelx: Increase number of decimal places in .ins file #2624

Merged
merged 6 commits into from Apr 16, 2024

Conversation

huwjenkins
Copy link
Contributor

Fixes #2621

Format of .ins file taken from screenshots in XPREP tutorials and matches the 3DED Biotin tutorial.

Copy link
Member

@dagewa dagewa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@graeme-winter graeme-winter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible, made one suggestion which I would be happy for you to ignore, thanks for the contribution :-)

@@ -156,13 +156,13 @@ def _write_ins(experiment_list, best_unit_cell, ins_file):
f"TITL {sg.type().number()} in {sg.type().lookup_symbol().replace(' ','')}\n"
)
f.write(
"CELL {:8.5f} {:8.4f} {:8.4f} {:8.4f} {:8.3f} {:8.3f} {:8.3f}\n".format(
"CELL {:7.5f} {:9.5f} {:9.5f} {:9.5f} {:8.4f} {:8.4f} {:8.4f}\n".format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should check the number of digits needed to express the unit cell and derive the format accordingly?

i.e. just saying if it does have a crazy unit cell like 10, 10, 1001, 90, 90, 90 we could probably drop the precision by 1dp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the correct behaviour is to check the esds to determine the precision required. I don't believe any of the SHELXx programs require fixed-width here? As mentioned I copied the formatting from some examples of XPREP output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can find online it seems that .ins files are read in free format by most (all?) shelx programs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so we should use format_float_with_standard_uncertainty() from dxtbx https://github.com/dials/dxtbx/blob/bb9337281e21a069cd57fd5c033fa1a1f8caf4a7/src/dxtbx/util/__init__.py#L35

to get the precision required?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think matching the XPREP output would be fine rather than adding extra complexity

@dagewa
Copy link
Member

dagewa commented Apr 16, 2024

There's a spurious ❌. Merging as-is, any future work on adaptive precisions could go in a new PR

@dagewa dagewa merged commit 77cda91 into dials:main Apr 16, 2024
9 of 11 checks passed
amyjaynethompson pushed a commit to amyjaynethompson/dials that referenced this pull request Apr 24, 2024
…file (dials#2624)

For `dials.export format=shelx` include one more decimal place in CELL and ZERR instructions, to match XPREP output
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.

dials.export format=shelx insufficient precision in esds for small molecule tutorial dataset
4 participants