Add showing of model connectivity #996

graeme-winter · 2019-11-03T10:12:27Z

Shows how models are related to experiments e.g.

Experiment / Models

Detector:
              0  1
Experiment 0  X  /
Experiment 1  X  /
Experiment 2  X  /
Experiment 3  /  X

Crystal:
              0  1  2  3
Experiment 0  X  /  /  /
Experiment 1  /  X  /  /
Experiment 2  /  /  X  /
Experiment 3  /  /  /  X

Beam:
              0
Experiment 0  X
Experiment 1  X
Experiment 2  X
Experiment 3  X

Seemed useful to me... make an option?

graeme-winter · 2019-11-03T10:20:00Z

N.B. allows clearer view of what options do e.g. with and without split_experiments in dials.index

Experiment / Models

Detector:
              0  1  2  3
Experiment 0  X  /  /  /
Experiment 1  /  X  /  /
Experiment 2  /  /  X  /
Experiment 3  /  /  /  X

Crystal:
              0  1  2  3
Experiment 0  X  /  /  /
Experiment 1  /  X  /  /
Experiment 2  /  /  X  /
Experiment 3  /  /  /  X

Beam:
              0  1  2  3
Experiment 0  X  /  /  /
Experiment 1  /  X  /  /
Experiment 2  /  /  X  /
Experiment 3  /  /  /  X

vs.

Experiment / Models

Detector:
              0  1
Experiment 0  X  /
Experiment 1  X  /
Experiment 2  X  /
Experiment 3  /  X

Crystal:
              0
Experiment 0  X
Experiment 1  X
Experiment 2  X
Experiment 3  X

Beam:
              0
Experiment 0  X
Experiment 1  X
Experiment 2  X
Experiment 3  X

graeme-winter · 2019-11-03T10:22:39Z

Does not show anything if len(experiments) == 1 as obviously the connectivity is trivial.

Should we have a limit for e.g. > 20 experiments or something? Can see this being useful for indexing > 1 lattice across > 1 sweep though.

Also welcome bikeshedding on X vs. / etc. - what are better symbols? - should we have --- and | to make ersatz ASCII art table? If you feel yes, please feel free to add 😉

rjgildea

Probably a useful addition, but at a risk of making the output of dials.show even more verbose than it already is (especially for multiple experiments, which is the intended use case of this PR). Perhaps it would be worth adding an option to switch it on/off (no strong opinion on the default)?
Could we add a test for this new output? I'm not sure that we have a multi-experiment test case for dials.show - probably a gap in the existing test coverage.

rjgildea · 2019-11-03T15:33:36Z

command_line/show.py

+    text.append("")
+    text.append("Detector:")
+    text.append("              " + "  ".join([str(j) for j in range(len(detectors))]))
+    for j, e in enumerate(experiments):


It might be better to use e.g. tabulate here instead of manually formatting the tables. We already use tabulate in xia2, so it should be available in a dials installation.

Don't know about tabulate however sounds like a good thing so 100% on board.

if you end up using dependencies in dials please add them explicitly to dials (ie. libtbx_config) even if they are already available indirectly

command_line/show.py

rjgildea · 2019-11-03T15:43:17Z

command_line/show.py

@@ -209,6 +209,8 @@ def run(args):
            )
        )

+        print(show_connectivity(experiments))


Not strictly a comment on this review, but a reminder that we should probably convert dials.show to use logging instead of print.

rjgildea · 2019-11-03T15:47:20Z

command_line/show.py

+    text.append("Detector:")
+    text.append("              " + "  ".join([str(j) for j in range(len(detectors))]))
+    for j, e in enumerate(experiments):
+        tmp = ["Experiment %d" % j]


I suspect if j >= 10 then it would mess up the table alignment - using tabulate should alleviate this.

graeme-winter · 2019-11-04T06:49:51Z

Test case data added to #995

graeme-winter · 2019-11-04T08:58:55Z

Added show_experiment_has_model bool param.

Re: tabulate; no idea; not learning but would very much welcome someone else dropping this in there modulo comments made by @Anthchirp re: dependencies

dagewa · 2019-11-04T09:45:40Z

tabulate looks interesting, but we have lots of uses of simple_table in the codebase at the moment. If we are going to include tabulate I suggest it is done outside this PR while converting the existing simple_tables for consistency.

rjgildea · 2019-11-04T14:44:07Z

command_line/show.py

@@ -26,6 +27,9 @@
 show_scan_varying = False
  .type = bool
  .help = "Whether or not to show the crystal at each scan point."
+show_experiment_has_model = False


Perhaps this would be a better parameter (and relevant function) name?

Suggested change

show_experiment_has_model = False

show_model_connectivity = False

graeme-winter · 2019-11-04T15:17:59Z

Am happy for anyone to take this one forward to close off :)

test/command_line/test_show.py

Anthchirp · 2019-11-04T15:51:29Z

test/command_line/test_show.py

+    expt = location.join("indexed.expt").strpath
+    result = procrunner.run(["dials.show", expt, "show_model_connectivity=True"])
+    assert not result.returncode and not result.stderr
+    assert "Experiment / Models" in result.stdout


Suggested change

assert "Experiment / Models" in result.stdout

assert b"Experiment / Models" in result.stdout

Anthchirp · 2019-11-04T15:54:28Z

command_line/show.py

+                if getattr(e, model) is m:
+                    row.append("X")
+                else:
+                    row.append("/")


Suggested change

row.append("/")

row.append(".")

my :bikeshed: contribution.

Call command line run() function directly instead of using procrunner.run().

X -> x / -> .

rjgildea · 2019-11-07T14:48:12Z

@graeme-winter I think dials.find_shared_models is relevant to this (although it doesn't appear to be tested) - perhaps given that similar functionality exists elsewhere then this PR is not needed?

$ dials.find_shared_models $(dials.data get -q l_cysteine_dials_output)/indexed.expt
DIALS (2018) Acta Cryst. D74, 85-97. https://doi.org/10.1107/S2059798317017235
DIALS 2.dev.1067-g89d664ba9
The following parameters have been modified:

input {
  experiments = /Users/rjgildea/software/cctbx/build/dials_data/l_cysteine_dials_output/indexed.expt
}

Number of sequences = 4
Sorting sequences based on timestamp
4 datasets collected on 2016-03-05
=====================================================  ====  ======  ==========  ============  ==========  ========
Sequence                                                 ID    Beam    Detector    Goniometer  Date        Time
=====================================================  ====  ======  ==========  ============  ==========  ========
/Users/graeme/data/i19-1-l-cys/l-cyst_04_#####.cbf.gz     0       0           0             0  2016-03-05  08:58:02
/Users/graeme/data/i19-1-l-cys/l-cyst_01_#####.cbf.gz     1       0           1             1  2016-03-05  09:01:45
/Users/graeme/data/i19-1-l-cys/l-cyst_02_#####.cbf.gz     2       0           1             2  2016-03-05  09:04:57
/Users/graeme/data/i19-1-l-cys/l-cyst_03_#####.cbf.gz     3       0           1             3  2016-03-05  09:08:09
=====================================================  ====  ======  ==========  ============  ==========  ========

dagewa · 2019-12-01T15:14:12Z

I think dials.show is a good home for this, but I prefer show_shared_models as a parameter name over show_model_connectivity Perhaps the behaviour of dials.find_shared_models and this PR can be merged, and dials.find_shared_models then retired?

graeme-winter · 2019-12-03T20:04:44Z

@dagewa show_shared_models - fine with this - I'll try to get this cleaned up though looks like we've acquired some conflicts along the way.

rjgildea · 2019-12-12T10:07:47Z

@graeme-winter any comment on #996 (comment)? It seems that this essentially does the same thing as this PR, albeit in a different form. If this PR is merged, then I suggest that dials.find_shared_models should be removed as it is then duplicate (and untested) functionality.

Anthchirp · 2019-12-12T10:14:46Z

I will spin that out as a separate issue to follow up at some point in the indefinite future

Add showing of model connectivity

52921ab

rjgildea requested changes Nov 3, 2019

View reviewed changes

Rename function; add phil param

1bae1ab

rjgildea added 4 commits November 4, 2019 11:58

Add test for show_experiment_has_model=True

9873e1e

Use tabulate to format tables

4580648

Test output of function without numerous levels of indirection

3b25090

De-duplicate code

b98ec94

rjgildea reviewed Nov 4, 2019

View reviewed changes

Rename experiment_has_model -> model_connectivity

841d097

rjgildea self-requested a review November 4, 2019 15:28

rjgildea approved these changes Nov 4, 2019

View reviewed changes

rjgildea requested a review from Anthchirp November 4, 2019 15:30

Anthchirp reviewed Nov 4, 2019

View reviewed changes

test/command_line/test_show.py Outdated Show resolved Hide resolved

Anthchirp approved these changes Nov 4, 2019

View reviewed changes

Anthchirp reviewed Nov 4, 2019

View reviewed changes

rjgildea added 4 commits November 4, 2019 15:56

.strpath not needed for procrunner

f9212af

stdout is binary data

e07fe3b

Skip level of indirection in test

57f546b

Call command line run() function directly instead of using procrunner.run().

:bikeshed:

4eaccaa

X -> x / -> .

rjgildea force-pushed the show_network branch from 45171c8 to 4eaccaa Compare November 6, 2019 15:02

graeme-winter mentioned this pull request Dec 3, 2019

DIALS 2.1 #993

Closed

Anthchirp added this to the DIALS 2.1 milestone Dec 4, 2019

Anthchirp added 3 commits December 12, 2019 09:19

Resolve merge conflict

8adc0d0

show_model_connectivity -> show_shared_models

acc59e1

incidental file cleanup

637082c

add newsfragment

b8b46f8

Anthchirp merged commit af519d9 into master Dec 12, 2019

Anthchirp mentioned this pull request Dec 12, 2019

merge dials.show show_shared_models and dials.find_shared_models #1070

Closed

Anthchirp deleted the show_network branch December 12, 2019 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add showing of model connectivity #996

Add showing of model connectivity #996

graeme-winter commented Nov 3, 2019

graeme-winter commented Nov 3, 2019

graeme-winter commented Nov 3, 2019

rjgildea left a comment

rjgildea Nov 3, 2019

graeme-winter Nov 4, 2019

Anthchirp Nov 4, 2019

rjgildea Nov 3, 2019

graeme-winter Nov 4, 2019

rjgildea Nov 3, 2019

graeme-winter Nov 4, 2019

graeme-winter commented Nov 4, 2019

graeme-winter commented Nov 4, 2019

dagewa commented Nov 4, 2019

rjgildea Nov 4, 2019

graeme-winter commented Nov 4, 2019

Anthchirp Nov 4, 2019

Anthchirp Nov 4, 2019

rjgildea commented Nov 7, 2019

dagewa commented Dec 1, 2019

graeme-winter commented Dec 3, 2019

rjgildea commented Dec 12, 2019

Anthchirp commented Dec 12, 2019

	show_experiment_has_model = False
	show_model_connectivity = False

	assert "Experiment / Models" in result.stdout
	assert b"Experiment / Models" in result.stdout

Add showing of model connectivity #996

Add showing of model connectivity #996

Conversation

graeme-winter commented Nov 3, 2019

graeme-winter commented Nov 3, 2019

graeme-winter commented Nov 3, 2019

rjgildea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme-winter commented Nov 4, 2019

graeme-winter commented Nov 4, 2019

dagewa commented Nov 4, 2019

Choose a reason for hiding this comment

graeme-winter commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjgildea commented Nov 7, 2019

dagewa commented Dec 1, 2019

graeme-winter commented Dec 3, 2019

rjgildea commented Dec 12, 2019

Anthchirp commented Dec 12, 2019