Overall interval_width_degrees #2268
Conversation
|
@dagewa thank you; noted; will review |
verbose but accurate 🙂 I wonder though whether that would worry people? |
|
That comes from cctbx. Not sure I can easily hide it as it is all in the option parser stuff |
|
Alternative would be to have a different named parameter and then not use the PHIL disambiguation mechanism, but overall I thought that seemed like a worse interface. |
There was a problem hiding this comment.
At first glance this does exactly what I had expected, so thank you. On more thorough investigation I think this does probably more than we would want? By default I would only expect the crystal parameters to vary, as the others (I had understood) were assumed to be static however... maybe I had that wrong?
Grey-Area screen19-2 :) $ dials.refine -c -e2 | grep interval_width
interval_width_degrees = None
interval_width_degrees = 36.0
interval_width_degrees = 36.0
interval_width_degrees = 36.0
interval_width_degrees = 36.0
interval_width_degrees = 36.0
This looks a lot like they are all scan varying? Since we cannot save scan varying detector positions etc. this confuses me some.
Anyway: requested change is to ensure that this default only applies to the crystal parameters... very happy to discuss this though.
newsfragments/2268.feature
Outdated
| @@ -0,0 +1 @@ | |||
| ``dials.refine``: use an overall ``interval_width_degrees`` parameter to set defaults for all models simultaneously. | |||
There was a problem hiding this comment.
Does this set it for all models?
For example I would not expect
refinement.parameterisation.detector.smoother.interval_width_degrees
to be affected?
There was a problem hiding this comment.
As in: I had expected that this would affect all crystal models
There was a problem hiding this comment.
With an additional sentence explaining the default behaviour here as discussed in the conversation for the PR looks great, thank you.
| .help = "Overall default value of the width of scan between checkpoints" | ||
| "in degrees for scan-varying refinement. If set to None, each" | ||
| "model will use its own specified value." | ||
| .type = float(value_min=0.) |
There was a problem hiding this comment.
Would we want to set a maximum value? 90° or something? Or is that an exercise for the user?
There was a problem hiding this comment.
Sometimes it is worth making a very low frequency scan-varying model, i.e. two checkpoints at the start and end. Then you capture essentially linear change in parameters. However, this is better done via the absolute_num_intervals parameters.
| @@ -790,6 +796,22 @@ def build_prediction_parameterisation( | |||
| A prediction equation parameterisation object | |||
| """ | |||
|
|
|||
| # Set overall default interval_width_degrees values, if requested | |||
| if options.interval_width_degrees: | |||
| options.beam.smoother.interval_width_degrees = options.interval_width_degrees | |||
There was a problem hiding this comment.
As comment above: I had anticiated this only applying to the crystal parameters. I would further mention right here that we cannot (IIUC) serialise scan varying beam, goniometer and detector parameters -> it is probably not correct to sweep them up in this?
There was a problem hiding this comment.
We can serialise scan-varying models other than crystals, but dials.integrate does not use them properly.
There was a problem hiding this comment.
actually, I might have fixed that at least for scan-varying beams. IIRC it is still problematic because things like the mosaicity model ignore the scan-varying parameters. So, while prediction in integration is scan-varying, the overall model remains global
|
What the new parameter does is to override the default values for all the smoothers, which are otherwise set to 36.0. The choice of whether to allow scan-varying parameterisation still remains with the models where each has a This all interacts with |
|
For small molecule data with P1 cell I sometimes do this: That way you get the usual static then scan-varying macrocycles, but the cell remains fixed to its static value and only the orientation refines scan-varying. This is because I found that the P1 scan-varying parameters often looked unreasonable (too much freedom) and I would not expect the cell to change anyway. Admittedly this is with ED data with narrow wedges and high centroid errors and would be less of an issue with good quality X-ray data |
Ah, in which case the effective behaviour is precisely what I would have expected thank you. Would then simply ask if this could be explicit in the news fragment. |
newsfragments/2268.feature
Outdated
| @@ -0,0 +1 @@ | |||
| ``dials.refine``: use an overall ``interval_width_degrees`` parameter to set defaults for all models simultaneously. | |||
There was a problem hiding this comment.
With an additional sentence explaining the default behaviour here as discussed in the conversation for the PR looks great, thank you.
Codecov Report
@@ Coverage Diff @@
## main #2268 +/- ##
==========================================
+ Coverage 80.48% 80.51% +0.02%
==========================================
Files 586 586
Lines 66899 66905 +6
Branches 8899 8900 +1
==========================================
+ Hits 53846 53868 +22
+ Misses 10992 10974 -18
- Partials 2061 2063 +2 |
Overall interval_width_degrees changes the defaults for each model. Fixes #2262
Set an overall
interval_width_degreesto adjust the defaults for all models.Fix for #2262.
By the way, you can also try
interval_width_degrees=Auto. No guarantee this gives sensible results in all cases, but it is most likely to work well for wide scans (full or multi-turn).