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

Export merged #182

Merged
merged 9 commits into from
Nov 12, 2021
Merged

Export merged #182

merged 9 commits into from
Nov 12, 2021

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Nov 5, 2021

This is intended to fix #179 but ended up more complicated than I expected. To get merged MTZs I had to get DUI to run dials.merge instead of dials.export. DUI takes a lot of control over what commands can be run and has various bits of special case handling, not all of which I understand.

There are still some bugs in this area (like #181 and #147) but I don't think these have been introduced here, just remain unfixed.

@dagewa dagewa requested a review from luisodls November 5, 2021 14:36
@dagewa
Copy link
Member Author

dagewa commented Nov 5, 2021

Ok, this is in a slightly better state now.

  • The option to select scaled intensities is only available if there is a dials.scale job up the tree
  • The option to select merged reflections is only available if scaled intensities are selected
  • The filename must now be chosen by a dialog that will additionally warn if going to overwrite

I could not figure out how to disable the Run button prior to a filename being selected. As a result, you still start with a default filename (integrated.mtz or scaled.mtz) and I left the red warning text in place in case this is going to overwrite a file. As a result you get warned twice if you use the dialog to select an existing filename. Oh well, to be sure, to be sure.

This needs a bit of testing over the next week.

@dagewa dagewa requested a review from ndevenish November 5, 2021 16:26
@dagewa
Copy link
Member Author

dagewa commented Nov 5, 2021

Oh, another thing: because the file dialogue allows the user to write the file anywhere they like, not just in dui_files/, the full path is now displayed. It might be better to disallow directory navigation in the file chooser (if I can figure out how). Writing outside dui_files/ might break other things (like i2 and Cloud integration)

No longer write output.json, but manifest.json, which tracks
full paths of MTZs, their types (integrated|scaled|merged) and
the associated report HTML
This was tricky the user could change the type of output MTZ, which
should update automatically set filename, like integrated.mtz,
scaled.mtz or merged.mtz. But this was not working. Simpler just
to revert to a default dui_output.mtz unless the user specifies
otherwise.
@dagewa
Copy link
Member Author

dagewa commented Nov 8, 2021

In discussion with Eugene and Kyle, I have now changed the MTZ copying behaviour, and recording of filenames in output.json, to a new mechanism that records file path and type (integrated|scaled|merged) in manifest.json. The automatic setting of filenames was partially broken and in the end I just removed it. The MTZ output name defaults to dui_output.mtz unless changed by the user.

@luisodls
Copy link
Contributor

luisodls commented Nov 10, 2021 via email

@dagewa
Copy link
Member Author

dagewa commented Nov 12, 2021

Thanks @luisodls. I'll merge now and make a release. I think DUI is working well enough now in my hands for the workshop

@dagewa dagewa merged commit 2c4a10d into main Nov 12, 2021
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.

Easily output merged MTZ
2 participants