Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#2343 closed defect (fixed)

ViewDockX up/down arrow actions should log the corresponding commands

Reported by: Elaine Meng Owned by: Conrad Huang
Priority: moderate Milestone:
Component: Surface/Binding Analysis Version:
Keywords: Cc: chimera-programmers
Blocked By: Blocking:
Notify when closed: Platform: all
Project: ChimeraX

Description

ViewDockX up/down arrow actions should log the corresponding commands. Currently they don't show anything in the Log.

Change History (25)

in reply to:  1 ; comment:1 by Conrad Huang, 6 years ago

I would imagine that would get really annoying really fast when your log 
fills up with "viewdock down name default_1" and "viewdock up name 
default_1".

On 8/22/2019 11:21 AM, ChimeraX wrote:

comment:2 by pett, 6 years ago

Cc: chimera-programmers added

I thought our paradigm was "actions in GUIs that affect the main graphics display need to log the equivalent command". Are we changing that? If so, model panel actions also produce a lot of logged commands. We would need to have a discussion as to when interfaces should log commands and when they shouldn't...

in reply to:  3 ; comment:3 by Elaine Meng, 6 years ago

I thought our plan for ChimeraX was to log all actions that change the display, or work towards doing so, and I’m strongly in favor of that.  It occurred to me that clicking in the table should also log the corresponding “display #!N models” commands.   People can remove consecutive duplicate commands from their command history if they want to.  This has long been a problem with Chimera, that there is no true record of all the stuff that you did.  The professional modeling software packages I worked with before coming to UCSF all had a complete log of all actions (other than mouse rotation/translation since that could be saved as “state”) including menu operations, and it frequently came in handy.
 Elaine

in reply to:  4 ; comment:4 by Conrad Huang, 6 years ago

We also said we don't want to flood users with messages of essentially 
identical actions, such as from moving a mouse.  I consider this fairly 
similar.  The "click-to-display" action will not generate identical 
commands.  The arrow keys might be okay but would be way too noisy for 
my taste.

On 8/22/2019 12:39 PM, ChimeraX wrote:

comment:5 by pett, 6 years ago

I guess I disagree that using an arrow key to examine a different ligand in the viewdock interface is in any way a continuous action. Actual researchers will want to examine the interaction in detail, perhaps turning on clashes, measuring distances and so forth. People actually examining docking results will not be using the arrow keys as if they were analogous to a slider.

Even for actual continuous actions such as mouse movement and sliders, the plan was to log a command on mouse up. I believe Tom intends to do this for threshold levels in the volume viewer, though he hasn't gotten to it yet, so that the threshold level actually in use is documented in the log if an image or session is saved.

At any rate, if we are varying from "GUI actions affecting the main display log commands" (which, like Elaine, I disagree with changing), then at the very least we need to come up with usable criteria for when commands _do_ get logged so that it isn't the Wild West out there with every developer deciding for themself.

comment:6 by Conrad Huang, 6 years ago

Status: assignedfeedback

All the viewdockx actions (that I noticed) now use commands instead of directly changing model attributes. The "Add Hbond" button now uses a different variant of the "hbond" command, so Phil's test case no longer results in zero hbonds. Anything else?

in reply to:  7 ; comment:7 by Elaine Meng, 6 years ago

The commands for up/down and hide/show look good to me, thanks!!  

I do have some reservations about the hbond/clash commands’ logic as to what is “the receptor”:  currently it is “everything else you didn’t specify in the viewdockx command” which is fine in the the most common situation of the only other thing being the receptor.  However, if you had multiple sets of ligands that you wanted to put in multiple instances of viewdockx, it is not good.  There is really no utility (or perhaps anti-utility) in the “name” option and allowing multiple instances of ViewDockX if this will be the way that receptor is defined.

Simplest solution is remove “name” option and ability to have multiple ViewDockX instances.  Another solution (avoiding heuristics) is to make the user specify both ligands and receptor, which I rather like because it is straightforward, e.g. something like:

viewdockx #1.2-11 receptor #1.1

…but that would require change in syntax.  Default receptor could be the current behavior of everything else. What do you think?

in reply to:  8 ; comment:8 by Conrad Huang, 6 years ago

For multiple ligands that you want in the same ViewDockX instance, you 
can use, for example, "viewdock #1,2.2-end".  If you're talking about 
independent sets of ligands, then there's no easy workaround; but I 
would argue that you probably want multiple instances of ChimeraX in 
that case.

Requiring specifying the receptor in the command is okay, but we should 
just compute the hbonds initially, as the receptor is /only/ used for 
that (at least so far).  It also means you cannot open the receptor 
after starting viewdock (but you can always close and restart viewdock).

Conrad

On 9/3/2019 9:52 AM, Elaine Meng wrote:

in reply to:  9 ; comment:9 by Elaine Meng, 6 years ago

Yes, I was talking about the ability you'd added to have multiple viewdockx instances with different names in the same instance of ChimeraX.  It doesn’t make sense with the current receptor definition for hbond/clash purposes.

I don’t understand your point about whether the H-bonds are calculated initially or when the button is clicked.  Why does that matter?

If the receptor were specified in the ViewDockX command, it could be shown as information in the dialog or its title bar.

However, none of this matters in the simple case of ChimeraX containing only one set of ligands and one receptor and one ViewDockX dialog.  If you only want users to work within this scenario, I should remove the documentation of the “name” option.

in reply to:  10 ; comment:10 by Conrad Huang, 6 years ago

The multiple instance capability was added before hbond, with the idea 
that you might have two different runs with the same receptor and want 
to look at them as separate groups.  Obviously adding hbond complicates 
that scenario.  Removing the multi-instance functionality is fine with 
me.  As I mentioned before, you can put all the ligands into the same 
viewdock instance.

My (unclear) point about computing hbonds immediately is that the user 
really does not need to specify a receptor /unless/ they want to 
calculate hbonds, as the receptor isn't used for anything else (so far). 
  So we may as well compute the hbonds immediately and add the columns 
on start up.  There are also the cases where (a) no receptor is opened 
yet, (b) each "ligand" is actually a receptor-ligand pair as in MORDOR, 
or (c) some other abuse of using a table with a set of models.

We can also change things so that if no receptor is given, clicking on 
the "Add Hbonds" button displays a (multi?-)model selector (whose 
selection viewdock will remember).  Or maybe we /always/ require the 
user to select the recepto, in which case the receptor need to be 
specified on the command line.

On 9/3/2019 10:45 AM, Elaine Meng wrote:

in reply to:  11 ; comment:11 by pett, 6 years ago

In a tangentially related point, I think the semantic model is that atomic structures that are sibling submodels of one another are supposed to represent alternative possibilities, only one of which is relevant at a time (in relation to other models) — such as NMR ensembles or sets of docked ligands.  So my feeling that, if possible, you should arrange the ViewDockX structures either as:

	A)  receptor: #N; ligands: #N+1.1-M
	B)  receptor: #N; ligands: #N.1-M

The hbonds code will handle case A as is.  I need to fix the code to handle case B — currently it treats all models under the same major ID number as “siblings” for the purpose of the interSubmodel keyword, but I think it should only treat models that also have the same parent tree that way.

in reply to:  12 ; comment:12 by Conrad Huang, 6 years ago

Is that really our semantic model?  What about assemblies of multiple 
molecules?  Aren't they siblings?

On 9/3/2019 11:10 AM, Eric Pettersen wrote:

in reply to:  13 ; comment:13 by pett, 6 years ago

Depends.  In Chimera, they were just one model.  In ChimeraX, they are sibling submodels of the assembly, each with their own “missing structure” pseudobond group and so forth.  So in Chimera, the sibling-model paradigm held fairly uniformly, whereas in ChimeraX there are exceptions.  Nonetheless, there is some semantic meaning to a model hierarchy, and lumping the receptor in with the ligands at the same level of the model hierarchy is not using the hierarchy to advantage.

in reply to:  14 ; comment:14 by Elaine Meng, 6 years ago

For ViewDockX, how about one of these:

(1) current behavior, “everything else” is receptor, except that I remove the documentation of the “name” option for multiple ViewDockX instances (could remove it from code too, but maybe that’s extra work).  This would not work for MORDOR where each ligand has its own copy of the receptor, but I don’t think anybody uses MORDOR.  

(2) Hbonds and Clashes buttons bring up model browser for specifying the receptor.  This still wouldn’t handle the MORDER situation, but as I said above...

My previous suggestion of specifying receptor in viewdockx startup command wouldn’t work with your current magic that “knows” what the docked ligands are when you start ViewDockX from the Tools menu.  I have no idea how that works.  I see your point now that if the receptor is given in the startup command, might as well also calculate H-bonds and clashes if there are no additional settable parameters.  However, if we ever added dialogs for their parameters like we had in Chimera, you wouldn’t want to calculate them automatically at the start.

For the model paradigm, I thought we never had #N and #N.N being actual models at the same time.  If there was an #N.N, then #N was just its container... Eric’s suggestion (B) just confuses me.  The receptor might just be opened from a separate PDB file.  We have no way of telling which model has the receptor relationship to the docked ligands.


in reply to:  15 ; comment:15 by Conrad Huang, 6 years ago

I'm happy with either one.  Obviously (1) is easier, even including code 
removal.  All of my examples are really cases where you wouldn't push 
the "Add Hbonds" button anyway, but I mentioned them for completeness.

On a different note, it appears that receptor models in maestro have the 
"b_glide_receptor" attribute, whereas ligands do not.  So I can actually 
create a ChimeraX model with N (I think N must be 1 according to 
Schrodinger) receptor models and a container model with all the ligands 
as children.  So with our example, #1.1 would be the receptor, and #1.2 
would contain #1.2.1, #1.2.2, etc.  Is that worth doing?  That would 
address Eric's semantic model issue.  It would also simplify "viewdock 
#1.2-end" to "viewdock #1.2", but that's relatively minor (in my view).

On 9/3/2019 11:43 AM, Elaine Meng wrote:

in reply to:  16 ; comment:16 by Elaine Meng, 6 years ago

I’m fine with (1) if nobody objects.  The documentation will need to bash people over the head with the message that everything that isn’t a docked ligand is treated as the receptor.  However, there’s a similar caveat even with model chooser or command-line specification of receptor, that one must exclude or delete solvent, co-crystallized ligands, etc. for the purposes of H-bond and clash calculations.

If there is a b_glide_receptor attribute, seems like it could/should be used when the ViewDockX tool is invoked from the menu (and maybe even if one specifies the entire model in the command) to exclude that model from the table of ligands.  Then if you opened Phil’s file and then started ViewDockX either from the menu or with “viewdockx #1” the table would not have the receptor in it.  I can’t think of any reason a person would want the model with b_glide_receptor=true listed in that table.  I’m neutral as to whether there should be any tinkering with the actual model numbers.


in reply to:  17 ; comment:17 by pett, 6 years ago

Organizing the receptor as #N.1 and the ligands as #N.2.1-M  would make hbonds work with default settings, and would make it easier to do things to ligands as a whole using the Model Panel (hide/select/color them) without affecting the receptor.  I think it’s a win.

—Eric

in reply to:  18 ; comment:18 by pett, 6 years ago

…make hbonds work with default settings **once I update the behavior of the interSubmodels keyword**

in reply to:  19 ; comment:19 by Elaine Meng, 6 years ago

The title of this ticket is done, but I’m confused by the other issues we’ve also been discussing in many comments of this ticket.

(1) I could have sworn that Conrad did already implement automatic organization of the models read from maestro to group the ligands as subsubmodels, e.g. receptor as #N.1 and the ligands as #N.2.1-M.  I’d even verified that by testing that  it was done.  However in the latest (9/10) daily build it is back to #N.1, #N.2, #N.3 … all at the same level.  Did I just hallucinate that this change was already done??

(2) Is it true that regardless of how the model numbers get assigned to these receptor+ligand maestro files, one will need to specify the ligand models at “viewdockx” startup (e.g. it will NOT work correctly if you just start the tool from the menu or use command “viewdockx” without specifying models)?   Should ViewdockX started from the Tools menu raise a dialog for specifying the ligand model numbers?

By "not work correctly," I mean that the receptor is treated as if it were one of the ligands and disappears when you click to the next ligand, and sets of atoms for finding H-bonds and clashes are not defined in the expected way.  In other words, if you just open the maestro file, start ViewDockX from the menu, and then click the H-bonds button, there is an error.  This is what happens now (9/10 daily build). Yeah, it’s sort of a user error but we are making it difficult for users.  Let me know if you want a ticket about it.  If you click the clashes button, it finds a bunch of clashes of the receptor with itself only which seems wrong given it’s using the “test others” options.  I’ll make a separate ticket for that.

If models are already marked as ligand or receptor in the Maestro file, then the viewdockx startup could automatically ignore all the model(s) marked as receptor for the purposes of what’s listed in its table.  But it does not appear to do that currently.  Should I make a ticket for that?

It is very difficult to document these issues in the ViewDockX page without referring to Maestro format which is not supported by the tool unless you go install something else.  

(3) Phil said it was possible that there might be multiple runs in the same file, if I understood correctly, one receptor, then a bunch of ligands that go with it, then another receptor, then another bunch of ligands that go with the second receptor.  Is there a plan for handling that case?  I’m not saying it’s worth making one yet, just asking.  Possibilities include making them into separate ViewdockX instances since Conrad has decided to keep that functionality.

 

comment:20 by Conrad Huang, 6 years ago

(1) should be done. Version 1.1 of Maestro has the model hierarchy code. Maybe you can check which version you have using "toolshed list"?

(2) If you have receptor and ligands under the same model number, you will need to explicitly choose the ligands when starting ViewDockX. The only time ViewDockX with no arguments works is if you open your ligands first and immediately start ViewDockX; if you open both receptor and ligands, then start ViewDockX, you will get the receptor in the ViewDockX list. If we want be perfectly clear while punishing users, we can require that they specify ligands on the command line.

(3) When I get a sample file, I'll start worrying about how to handle that case.

in reply to:  21 ; comment:21 by Elaine Meng, 6 years ago

(1) Sorry, my bad. I keep forgetting it is not a feature of ViewDockX current in the daily build but some other thing I have to get/update from Toolshed. 

(2) It all hinges on how many people actually do it the way you are hoping they will, vs. how many people won’t  do it that way and then get all mixed up or think ViewDock doesn’t work.  I think the latter camp will be large, as mentioned/addressed in #2402.  I already fell into that camp once myself, after working on other projects and then trying to get back into using ViewDockX.

(3) fine, just mentioning for completeness because Phil mentioned it.

comment:22 by Conrad Huang, 6 years ago

You're mistaking the behavior of "viewdock with no arguments" as being "smart". Viewdock command syntax is that you can optionally provide a modelspec. When none is specified, then all models are made part of the viewdock list. If you want different behavior, come up with a different command/subcommand.

in reply to:  23 ; comment:23 by Elaine Meng, 6 years ago

Well, it works fine now if I open a receptor PDB and then open a separate ligand file such as the example in the User Guide page, and then use command “viewdock” with no arguments (or start ViewdockX from the Tools menu).  Apparently it already knows to ignore the #1 receptor.  My suggestion is to be similarly easy-to-use in the Maestro combination file situation.

comment:24 by Conrad Huang, 6 years ago

Resolution: fixed
Status: feedbackclosed

Okay, receptors from Maestro files no longer get ViewDock attributes assigned, so they are ignored even when included in atomspec. So "viewdock #1" should work properly after you open a Maestro file. Even "viewdock" might work, depending on what other models are present.

You need to update to Maestro version 1.3 to get this behavior.

in reply to:  25 ; comment:25 by Elaine Meng, 6 years ago

Woohoo, works great!!   All the easily confused or lazy users (OK, that means me) sincerely thank you.  I guess this is really ticket #2402, which I’ll close if you haven’t already.
Note: See TracTickets for help on using tickets.