dermen / sim_erice Goto Github PK

Need to add the following at the top of sim_view.py when diffBragg is built using kokkos

from simtbx.kokkos import gpu_instance
kokkos_run = gpu_instance(deviceId=0)

Closing the simulation window should also exit the program

@nksauter @dermen There's a bug in the scaling of diffuse intensity in cctbx diffuse_util.h and diffuse_util_kokkos.h. I'm about to push a revision to the cctbx_projects repo. Please rebuild using the revision.

Subtask of #16

Here's another bug possibly related to #36 and #12. Start the viewer

Click in the text box of one spinner control. The label turns blue.
Now click on the up/down arrow keys of a second spinner control. The label of the second control does not turn blue.

The way it should behave is that the second control should now turn blue and active.

After randomizing orientation (once or any number of times), only monochromatic spectrum mode produces an image. This is not fixed with the "reset all" button.

Currently there are two sets of callbacks, one changing the parameter under focus due to mouse click, and one that changes the focused parameter due to Left/Right arrow. These actions be strictly pinned together. There should be only one focused parameter, not two independent focus mechanisms.

As a symptom, the domain_size small step is twice the value initialized in the code. Double check that fixing the focus mechanism also fixes the size of the small step.

Problem statement: If the current value has trailing 9's (like "14.499999999999999999999") it can be extremely difficult to recover control to type in a new value. Instead, each control should define by default an allowable number of decimal places, and then round the current value to the pre-set (using the Python round() function).

Problem statement: The U0 angles are not updated when a new PDB ID is loaded.
When a new PDB code is requested, the U0 should either be reset to the identity matrix (probably preferred), or the old U0 orientation should be applied to the new crystal model (might potentially be argued).

Per @mewall Increasing the anisotropy factor to 3 decreases the value of gamma along one axis which leads to some visible streaked diffuse features. For this reason, I would recommend changing the default anisotropy factor to 3 in the viewer.

Laue group may not be passed correctly in rotation mode. Set this correctly in rotation mode.

It is hard to see the Bragg peak above the fuzzy diffuse halo. Perhaps there could be an option to allow a heat map to better portray steep pixel-to-pixel intensity changes. Activated by a keyboard shortcut?

There's a tkinter bug thrown when using the arrows to adjust a parameter in sim_view.py. It looks like tkinter might have changed the argument list for one of the calls. Either downgrade tkinter or change the call in the code.

Per request from the reviewer let's have a distance phil parameter to set the detector distance, at expert_level=1. Possibly the pixel size should be a phil parameter too.

Good initial implementation but lots of work to do in the next few days. Some observations to start with:

• 1) Running the script with no parameters gives helpless traceback:
  Exception ignored in: <function command_stream.del at 0x7f2a1cef6160>
  Traceback (most recent call last):
  File "/pscratch/sd/n/nksauter/edge/alcc-recipes/cctbx/modules/cctbx_project/iotbx/shelx/lexer.py", line 35, in del
  self.file.close()
  AttributeError: 'command_stream' object has no attribute 'file'
  Traceback (most recent call last):
  File "/pscratch/sd/n/nksauter/edge/alcc-recipes/cctbx/modules/sim_erice/sim_view.py", line 306, in
  frame = SimView(root, params, pdbfile)
  File "/pscratch/sd/n/nksauter/edge/alcc-recipes/cctbx/modules/sim_erice/sim_view.py", line 43, in init
  sg = str(symmetry.space_group_info())
  AttributeError: 'NoneType' object has no attribute 'space_group_info'

• 2) Would like program to use its own PDB file if none is provided

• 3) Attempts to get command line help ("-h") should work

• 4) Help text could include the ability to download PDB files with "iotbx.fetch_pdb"

• 5) Please let's have the beam center near the lower left corner as in the original simulator, so the limited screen real estate can be more focused on higher-angle scattering.

• 6) Please have each unit cell parameter independently adjustable.

• 7) The screen readout for missetting angles could not possibly be in degrees! Something is wrong with the units.

• 8) Some of the parameters need much finer granularity per arrow click. For example, the mimimum mosaic angle is 0.3 degree, but it should be continuously adjustable down to 0.0.

• 9) Adjusting the MosDom size scales the intensity up. This should not be the behavior of the simulator. Instead, the simulation should treat the overall crystal size as constant, so the spot intensities are always roughly on the same scale, and visible to the corners. The MosDom parameter should more-or-less be inversely coupled with a hidden "number-of-domains" parameter such that the overall crystal size remains constant.

@mewall can you confirm the viewer has the correct value for diffuse anisotropy (1.0) or should it be something else?

When I hit enter after adjusting a parameter, sometimes its unclear what happened, if anything..

I think it would be good to have some kind of text visible in the viewer that indicates the parameters were accepted, and that nanoBragg was run successfully.. We could even have a button that pops up a window showing the internal state of nanoBragg e.g., text output of show_params().

One of the params_num/params_cat is the result of committing non-functional code that is still work in progress. Prefer to keep such code out of master branch with selective use of "git add --patch".

Diffuse streaks appear not to be oriented strictly along unit cell axes. Possibly the behavior is related to whether or not the Gamma matrix is expressed in Miller index units?

I'm seeing an issue where the same domain size setting yields different images for otherwise identical control settings

o Open sim_view and immediately load PDB ID 1cm1 (calmodullin)
o Randomize orientation a few times until an image appears where the spots are very elongated. Remember how this looks.
o Increase the domain size to 2000 and decrease it back down to 1000. The image looks different (and perhaps more reasonable)

Let's make sure we are calculating our orientation according to the accepted practice, see David Waterman's 2016 paper at https://doi.org/10.1107/S2059798316002187, equations 12-13:

Here, the B matrix is an expression of the unit cell parameters in a standard orientation. U is the rotation matrix returned by the "Randomize Orientation" button. RxRyRz are small rotations around lab axes entered into the Rotation x,y,z spinner controls. The resulting matrix is "A" which is further modified by R-phi which is the rotation about the goniometer spindle.

If there is goniometer rotation in the sim_viewer we should indicate which axis is the spindle.

@irisdyoung The Reset all button currently does not return the crystal to the same orientation as at startup. It's not clear which orientation is chosen, probably U0 should just be set to the identity.

@irisdyoung please remove the constraint that switches to Monochromatic when the user chooses Diffuse. Simply keep whatever spectral mode is currently active.

Command line request for help fails:
command_line> DIFFBRAGG_USE_CUDA=1 libtbx.python sim_view.py --help
Traceback (most recent call last):
File "sim_view.py", line 918, in
frame = SimView(root, params_num, params_cat, pdbfile)
File "sim_view.py", line 370, in init
sg = str(symmetry.space_group_info())
AttributeError: 'NoneType' object has no attribute 'space_group_info'

Subtask of #16

Subtask of #16

I've seen the Rot-z up arrow stick, and it just keeps generating new images while incrementing the rotation. There is no way to recover. @mewall speculates it is "just a Tk problem", but could this be fixed by more advanced function calls?

An analogy. In college electronics there is a problem with simple on/off switches. There is a zone right in the middle where the switch keeps bouncing between "on" and "off". The solution is to hook up an operational amplifier that debounces the circuit and allows smooth operation.

@dermen look more carefully as to how to allocate granularity between mosaic domains and oversampling.

Use this: °
Not this: º

Let's give the user the option of entering a PDB code (with automatic download of the file behind the scenes), or simply to give a PDB file name on the local file system. It would be good if the unit cell and space group could be displayed.

As far as I can see, all the spinner controls are broken in the following sense.
Start the viewer. Single click inside the text box of the Rotation(z) control. Now repeatedly click on the up-arrow.
Even though the value increments, there is no refresh of the picture.

@mewall could you help identify a good starting value for diffuse sigma? The current default of 0.4 gives a weak signal, and I would prefer to show a clearer effect when the diffuse features are first toggled on.

@dermen reports clicking in a text box of one of the spinner controls, and then being unable to use the left/right arrow keys to edit the text in the box. Instead, the arrow keys navigate to different spinner controls. This "feature" should be removed, and the keys should be used for normal text editing as Derek had expected. The corresponding documentation also needs updating in sim_viewer.py and README.md.

Since it is a binary choice, no need to make the user look at a pull down menu. Would a push button suffice?

The term "reference" is ambiguous. It appears in two places "Overlay with reference" and "Update reference image". Try to be more precise. Perhaps something about "pinned model"

Problem statement: in the module on_the_fly_simdata.py, the randomize_orientation() function call returns a new orientation matrix. However, as far as I can see, the new orientation is not uniformly distributed in rotation space. This is because the rotation (rot) is drawn from a gaussian rather than a uniform distribution. Fixing this should be simple. First, the mersenne_twister instance should be put in the global scope so that it returns a persistent sequence of random numbers. Second, the uniformly distributed orientation should be

return mersenne_twister.random_double_r3_rotation_matrix()

This may be relevant to issue #32 (I'm not sure, it requires investigation).

Per @mewall keep the program behavior as is but make some recommendations to the user about investigating diffuse scattering in some documentation: (1) speed things up by using Monochromatic spectrum shape; (2) increase domain size to 10000 Angstroms to more easily see what happens when changing diffuse scattering parameters.

Per request from the reviewer let's set these phil parameters:
mosaic_domains_bragg=100
mosaic_domains_diffuse=10

Problem statement: the way it is now GPU acceleration is only accessed if an environment variable is set:

DIFFBRAGG_USE_CUDA=1 simtbx.sim_view
DIFFBRAGG_USE_KOKKOS=1 simtbx.sim_view

As a user-friendly interface to diffBragg, the simtbx.sim_view command should be given default behavior that aligns with our current intended use case, namely always use Kokkos. Please implement this. Any number of options might work, but the simplest is just for the sim_view.py script to set the environment variable as soon as it starts. We need this done before returning the manuscript to the editor.

Let's have a concise explanation for the keyboard shortcuts printed on the control panel. Be sure to include the L/R arrow, U/D arrow, and shift+U/D arrow. Also the other shortcuts listed in the -h help string.

Find a way to allow 2x and possibly 4x zoom. Mouse wheel or keyboard shortcut? Zoom in on current mouse position? When Zoomed, optimize performance by calling diffBragg on smaller region of interest. Hit escape to return to normal view.

Set important hyperparameters in Phil, to expose them for power users. Parameters should include mosaic_domains, oversampling, total rotation width, and perhaps others.

Enclose the run() method in the appropriate initializers and finalizers.

Can't figure out how to add a new developer. Someone else knows?

Currently two defaults are used, 100 domains for Bragg-only and 1 domain for Bragg+diffuse. Change this to one default only covering all cases.

Active diffuse scatter and set diffuse anisotropy to 3.0. Bragg spots are now surrounded by diffuse halos oriented approximately vertically.

Now repeated calls to "Randomize orientation" always preserve the vertical orientation of the halos. This is contrary to the expected behavior with the shape of the halo tied to the lattice.

In rotation mode, the control panel should illustrate a vector representing the spindle axis. Is the axis horizontal?