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

Allow dials.predict to work without images #2553

Merged
merged 15 commits into from
Mar 13, 2024
Merged

Allow dials.predict to work without images #2553

merged 15 commits into from
Mar 13, 2024

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Nov 13, 2023

If we just want to predict for some geometry, without having image data, we need check_format=False

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #2553 (70a5482) into main (c6bf4cd) will increase coverage by 0.10%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2553      +/-   ##
==========================================
+ Coverage   78.49%   78.59%   +0.10%     
==========================================
  Files         609      609              
  Lines       74853    75385     +532     
  Branches    10674    10816     +142     
==========================================
+ Hits        58753    59250     +497     
- Misses      13937    13956      +19     
- Partials     2163     2179      +16     

dagewa and others added 7 commits November 30, 2023 14:19
* Fix dials.refine program documentation.

dials.refine overrides a couple of PHIL parameters, so make sure
we take the final PHIL scope used by the program.

* News

Fixes #2542
…sformation in symmetry analysis (#2552)

Try to print more useful log output and suggested workarounds and exit cleanly.
Fixes #2355
Fixes #2130
orrectly output imageset id when joint_indexing=False and max_lattices>1 in dials.index
Updated refinement working set of reflections threshold. Updated tests to reflect this, and a change in precision for Scan oscillation.
Updated tests to reflect Scan changes from dxtbx #620, and updated test file from dials-data #447.
@dagewa
Copy link
Member Author

dagewa commented Dec 12, 2023

Actually setting check_format=False breaks the (untested) ignore_shadows=False option, added for #349. In 863044f it was noted that we really need a way to set check_format=auto (and the relevant issue is tracked here cctbx/dxtbx#187).

There is currently no good way to do this other than having a fallback parser

@rjgildea
Copy link
Contributor

rjgildea commented Dec 12, 2023

There’s a pattern/workaround used in dials.show to reinstantiate the experiment object with check_format=True when required for options that need access to the image - similar could work here?

experiments.as_json(), check_format=True

@dagewa
Copy link
Member Author

dagewa commented Dec 13, 2023

Oh my, there's a feeling of déjà vu... Yes, I'll try this. Thanks!

the experiments with check_format=True at the point they are
needed.

Thanks @rjgildea for the reminder!
@dagewa
Copy link
Member Author

dagewa commented Dec 13, 2023

@graeme-winter added you for review as this touches on the shadow filtering stuff that you added. I think this needs a test case. Can you suggest a good one? (otherwise I guess I can use almost anything just to exercise the mechanics of it)

@dagewa
Copy link
Member Author

dagewa commented Feb 1, 2024

@graeme-winter I tried testing this with any old data set:

dials.predict $(dials.data get -q centroid_test_data)/indexed.expt ignore_shadows=False
The following parameters have been modified:

ignore_shadows = False
input {
  experiments = /home/fcx32934/sw/cctbx/build/dials_data/centroid_test_data/indexed.expt
}

Traceback (most recent call last):
  File "/home/fcx32934/sw/cctbx/build/../modules/dials/src/dials/command_line/predict.py", line 141, in <module>
    run()
  File "/home/fcx32934/sw/cctbx/conda_base/lib/python3.10/contextlib.py", line 79, in inner
    return func(*args, **kwds)
  File "/home/fcx32934/sw/cctbx/build/../modules/dials/src/dials/command_line/predict.py", line 137, in run
    script.run(args)
  File "/home/fcx32934/sw/cctbx/build/../modules/dials/src/dials/command_line/predict.py", line 118, in run
    shadowed = filter_shadowed_reflections(
  File "/home/fcx32934/sw/cctbx/modules/dials/src/dials/algorithms/shadowing/filter.py", line 19, in filter_shadowed_reflections
    shadow = masker.project_extrema(
AttributeError: Please report this error to dials-support@lists.sourceforge.net: 'NoneType' object has no attribute 'project_extrema'

Seems to me that the shadowing stuff you added is broken? This error is the same on main.

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 like a sensible change set, thank you, highlighted one area for API improvement for ExperimentList but that can wait for another day

if not params.ignore_shadows:
try:
experiments = ExperimentListFactory.from_json(
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me think we need a better way in the API of making this change

Not suggesting that we change it here just that the ExperimentList needs to have an add_format() or something

@dagewa
Copy link
Member Author

dagewa commented Feb 7, 2024

Thanks @graeme-winter. I don't need this merged urgently, so I wonder if we should look at the failures with ignore_shadows=False (#2553 (comment)) in this PR, or spin it out as a separate issue?

@dagewa dagewa merged commit 4a251d1 into main Mar 13, 2024
18 checks passed
@dagewa dagewa deleted the predict-no-data branch March 13, 2024 13:22
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.

None yet

6 participants