dials.import: individually select models from reference expt

[dwpaley]

Add new phil parameters input.use_beam_reference, use_gonio_reference, and use_detector_reference. If set to false, the corresponding models are not imported from the reference geometry file.

[codecov]

Codecov Report

Merging #1371 (4adeece) into master (5584654) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1371      +/-   ##
==========================================
+ Coverage   65.72%   65.73%   +0.01%     
==========================================
  Files         615      615              
  Lines       69122    69151      +29     
  Branches     9550     9553       +3     
==========================================
+ Hits        45429    45459      +30     
  Misses      21854    21854              
+ Partials     1839     1838       -1     

[graeme-winter]

We discussed this over at dials-east yesterday - the idea is fine but we felt that the proposed command-line API was cumbersome. A suggestion was to use

reference.models=beam,goniometer

or

reference.models=all

as the default - by analogy with fixing parameters in refinement.

However it's also noted that there is a lot of inconsistency across dials with how command line options like this are presented. @dwpaley how would you feel about this alternative command line scheme?

[ndevenish]

Probably the place most likely to encounter this pattern is in refinement where it's used to turn on/off refinement parameters e.g.

beam {
      fix = all *in_spindle_plane out_spindle_plane *wavelength
        .type = choice(multi=True)
}

[dwpaley]

I think that sounds great. Will have to rewrite the test and get back to you.

[dwpaley]

Thanks, @graeme-winter and @ndevenish. It's now a single phil entry using type=choice(multi=True) as in Nick's example. What do you think?

[ndevenish]

So, I know we sort of asked for it, but I'm really not sure how well suited the type=choice(multi=True) is - I didn't realise that it was quite so confusing - as in, reference_models=beam chooses beam but reference_models=beam detector chooses neither - needing to point out that there is a trap in the test cases doesn't really help users. I don't think we understood that was how this worked in refinement either, where it's normally only used for beam.fix=all (which works as expected).

So, given that this parameter is probably expected to be used for turning on/off single I'd lean towards either the original solution (sorry!) or something custom that just takes a multiple=True and custom splits on space/comma, but the benefit needs to be weighed against other uses of this. I'm not sure where else we might have user-facing multiple option choices like this.

I'm leaning towards the original multiple parameters, which is a bit of a mouthful but easy to understand and explicit.

Thoughts?

[dwpaley]

Hi Nick, thanks for your comments. I agree that your example is confusing, and that clunky + explicit is better. I think the first commit is still ok if we just get rid of the others. Is that what you were thinking?

[ndevenish]

Yes, I would be fine with that. It's explicit enough that we could always add a combined parameter later, if we were really eager. @graeme-winter fine with this?