sgsaenger / vipster

Visualization and editing of periodic molecular structure files.
https://sgsaenger.github.io/vipster
GNU General Public License v3.0
24 stars 7 forks source link

[REQUEST] A way to draw bonds between atoms (and select bond type explicitly) #33

Closed jewettaij closed 5 years ago

jewettaij commented 5 years ago

Is your feature request related to a problem? Please describe.

For coarse-grained modeling, it is important to be able to manually specify which pairs of atoms are bonded together (instead of trying to infer it from the distance and atom-type names). I know we have discussed this before by email, but I'm making a formal request for it here. I hope that's okay.

Describe the solution you'd like

Would it be possible to have a "New Bond" feature? (Perhaps select "Edit"->"New Bond"?). Then select two atoms?

Additionally, it would be pretty useful to be able to display a (hideable) table of bonds. (Similar in style to the hideable tables of atoms and cell vectors that you already have on the left-hand side.) This way, the user can also specify the bond type (a text string).

AtomID1  AtomID2  BondType
    143       534      C-H
    685       349      C=O
     :

(For most coarse-grained models, the angles, dihedrals, and impropers can be generated automatically from the bond network and atom types. So this would be enough to help most users.)

To make VIPSTER work with coarse grained models, you would also have to provide a way to override VIPSTER so that it no longer automatically tries to infer the atom's type (element) from its name. ("C3" is assumed to be a carbon atom. "Ha" is assumed to be a hydrogen atom. Etc...) Perhaps I should make a separate feature request for this.

Feel free to ignore this feedback. -andrew

sgsaenger commented 5 years ago

This is more like three and a half feature requests, so let's go:

Setting bonds manually: Planned, but not started yet. I'd envisioned it as an additional mouse-mode to reduce the number of clicks necessary.

Bond table: Great idea, could tie in pretty well with manual bond setting. Probably as a tool-widget.

I'm not sure whether it's a good idea to introduce a separate bond type, though. So far, bonds don't have a type at all, only two atoms and a length (for convenience). Admittedly, this is based on the original use-case of automatic-only bonds. Also, I have zero experience with coarse-graining. What behavior/benefits do you expect from this feature? If the atom-types are specified for a bond, what should happen if bonds are in manual-mode, but an atom-type changes? Sure, we can overwrite the user's changes, but that sounds frustration-inducing while not providing obvious advantages. Distinguishing between single/double bonds had been explicitly decided against as it's a tremendous amount of work, while I couldn't find any benefits so far. Please share your view!

Inferred types: This is definitely an area that ties in with #32... There's a few usability problems so far regarding editing and saving the global table, but I think the mechanism behind is pretty solid. Both the molecule's periodic table as well as the global table can be edited to your liking, so I see only a problem of ease-of-use and not of not-fit-for-the-task. If there's a certain "atom type" you'd like to use (be it in coarse-graining or with a certain classical force field), I imagine the work flow as such:

What I think is missing now are:

Does this sound reasonable?

jewettaij commented 5 years ago
This is more like three and a half feature requests, so let's go:

(Yep. Sorry about that. I was a bit lazy.)

Setting bonds manually:

Planned, but not started yet.
I'd envisioned it as an additional mouse-mode to reduce the number of clicks necessary.

Sounds great.

Bond table:

I'm not sure whether it's a good idea to introduce a separate bond type, though.
So far, bonds don't have a type at all, only two atoms and a length (for convenience).
Admittedly, this is based on the original use-case of automatic-only bonds.
...
What behavior/benefits do you expect from this feature?

Currently VIPSTER can create LAMMPS DATA files. (I have tested this and it seems to work nicely when I tried it.) In a LAMMPS data file, every line in the the "Bonds" section of that file contains 4 numbers:

If you decide to include a bond-table, then I propose providing an option to display an extra column in that table (the 3rd column. The first two columns would be for the atom ID numbers.)

VIPSTER must somehow already guesses the bond's type integer when it creates the LAMMPS DATA file. The purpose of this 3rd column is to allow users to manually specify what this bond-type is. (For LAMMPS output, it should be an integer. If we decide to add MOLTEMPLATE output, then it should be a string.)

If the atom-types are specified for a bond, what should happen if bonds are in manual-mode, but an
atom-type changes?

The user could leave the 3rd column blank or fill it in. When the user neglects to specify a bond type, I suggest that VIPSTER should continue using whatever method it currently uses to decide the bond type. In my mind, this should happen regardless of whether the user changes the atom types. (I assume VIPSTER currently generates a bond-type integer by hashing the two atom-type strings in some way.)

Sure, we can overwrite the user's changes, but that sounds frustration-inducing
while not providing obvious advantages.

If the user doesn't want to override the way VIPSTER chooses bond-types, then they could choose to leave this column blank (or not to even show the 3rd column). I agree that most of the time the user would not need to enter the bond type (and hence the 3rd column should be hidden by default). Having a 3rd column is useful whenever the user wants to build a coarse-grained model. This is because the atom type names will probably not be in the periodic table, so automatic-bond-length assignment is not possible. (That is, unless you provide a mechanism for the user to manually specify bond-lengths and/or bond-types for different pairs of atom types. More on that topic below.)

Distinguishing between single/double bonds had been explicitly decided against as it's a tremendous
amount of work, while I couldn't find any benefits so far.  Please share your view!

Yes. Sorry. I agree. It's not VIPSTER's responsibility to infer whether a bond is single or double. My apologies for the confusion. I just chose "C-H" and "C=O", or "ca'_n4" as examples of typical strings which the user might type in to the third column of the table. (...And it would only make sense for the user to provide strings for the bond types, instead of integers, if VIPSTER exporting to a 3rd-party program such as moltemplate which can interpret these strings. Otherwise the 3rd column might only accept only integers, or blanks.) Again, most of the time, the user would probably leave the 3rd column (bond-type) blank.

Inferred types: I'll comment on this in the next post (later today)...

jewettaij commented 5 years ago

Inferred types:

This is definitely an area that ties in with #32...
There's a few usability problems so far regarding editing and saving the global table,
but I think the mechanism behind is pretty solid.
Both the molecule's periodic table as well as the global table can be edited to your liking,

Are you talking about the "PSE (Molecule)" and "PSE (Global)" tables? I will assume you are. (I discovered these after I wrote my previous reply.)

If there's a certain "atom type" you'd like to use (be it in coarse-graining or
with a certain classical force field), I imagine the work flow as such:
  -Create your "atom" and assign the type
  -Edit the molecule's periodic table to make the "atom" behave as you wish
  -If you recognize that you'll reuse this type, add it to the global table

I did not realize that VIPSTER had this capability, and I agree that having a way for users to edit the molecule table is useful. It might solve all of the issues. Excerpt:

What I think is missing now are:
  -The possibility to save a molecule's type back to the global table
  -Overwrite the molecule's type with the base-values
  -Add arbitrary types manually to the global table
Does this sound reasonable?

Perhaps... Coarse-grained users will have no use for the existing global periodic table, since most coarse-grained particles are much larger than single atoms. (They range in size from heavy-atoms, amino-acid side-chains, whole proteins, colloids, swimming bacteria, or perhaps one day even swarms of living creatures with internal logic. It is important to be able to define bonds between any pair of particles, regardless of whether they are atomic elements or something larger. It sounds like you realize this. To make sure we understand each other, I have a couple questions:

If a user does not want VIPSTER to automatically create bonds between nearby atoms, I assume they can currently edit the local table for that bond type, and set its "Bond cutoff radius" to zero. Then later users could manually add bonds between these atoms (if you decide to add that feature). Is that correct? If so, then this sounds good to me. I'm happy as long as there is some way for users to turn off (or override) automatic bonds for some atom types. It sounds like the only thing that's missing from your list would be the ability for users to be able to manually define bonds between pairs of atoms.

Whew. Thanks for addressing all my questions in this sprawling, disorganized feature request.

sgsaenger commented 5 years ago

VIPSTER must somehow already guesses the bond's type integer when it creates the LAMMPS DATA file. The purpose of this 3rd column is to allow users to manually specify what this bond-type is. (For LAMMPS output, it should be an integer. If we decide to add MOLTEMPLATE output, then it should be a string.) ... The user could leave the 3rd column blank or fill it in. When the user neglects to specify a bond type, I suggest that VIPSTER should continue using whatever method it currently uses to decide the bond type. In my mind, this should happen regardless of whether the user changes the atom types. (I assume VIPSTER currently generates a bond-type integer by hashing the two atom-type strings in some way.)

Atm, the bond type for LAMMPS will be generated from the pair of bounded atoms when the file is written, simple as that. Vipster itself is only interested in the pair of bounded atoms for a) determining the bonds and b) for coloring the bonds when drawing. Drawing obviously also needs the length, so it is cached. The problems I see when bonds are (manually) typed are:

Admittedly, I've been biased by our use of a few classical forcefields that define bond types in the same way. Students have come up with the question of specifying bond types, but it was easy to shut them down when this is the way we would do it, anyways :wink: It boils down to this: If we have an optional widget with an optional feature to optionally set a bond's type, this shouldn't be able to result in an invalid file. And if it can be helped, also not in a wrong file.

Please don't take this too negatively! This list of problems is more for bookkeeping purposes than for shutting down the idea. I'll be thankful for all feedback.

sgsaenger commented 5 years ago

Are you talking about the "PSE (Molecule)" and "PSE (Global)" tables? I will assume you are. (I discovered these after I wrote my previous reply.)

Yes. On the testing branch I've already (finally...) removed the german abbrevations.

* Is it correct to assume that the local (Molecule) table overrides the global table?  For example, in some coarse-grained models of proteins, each particle represents an amino acid.  In these models, the "H" and "P" particles correspond to "hydrophobic" and "hydrophilic" amino acids, respectively.  If I create a molecule containing particles named "H" and "P", will VIPSTER confuse these with atomic elements hydrogen and phosphorus?  If so, that's fine as long as the user can override this by editing the local/molecule table to define the properties of the "H" and "P" particles.  (I don't mind asking the user to manually edit the local table.  That seems quite reasonable.)

Correct!

* How do you envision that a user could save the local ("molecule") table so that they could use it with new molecules that they want to build later?  (Is this what you mean when you say "save a molecule's type back to the global table"?)  I agree that there should be a way for users to save this local table.

* What do you mean by "Overwrite the molecule's type with the base-values"?

* Could custom "local" tables be used to store common defaults for popular force-fields (like OPLSAA or AMBER/GAFF2)?  Could they be loaded from a file?

ATM, if local[H] is not present, it will copy global[H]. If local[HA] is requested, it will also copy global[H] due to the fallback algorithm. What is planned for the near future:

Loading a custom table from a file is a neat idea! I'll definitely look into it. Currently, i'm waiting for a reliable implementation of C++17's filesystem on the main target platforms to rework the config file handling. I've already split the different parts into multiple files on the filesystem branch, it's just not merged due to mingw/xcode issues. When this can finally be merged, I'll look into e.g. command line switches to specifiy the periodic table file.

Whew. Thanks for addressing all my questions in this sprawling, disorganized feature request.

Thanks for the constructive discussion!

jewettaij commented 5 years ago

You provided some great thought experiments which demonstrate why part of my proposal (ie being able to manually specify the bond types) is a complicated feature. I myself did not realize how complicated it would be. But it is possible.

However I recommend that you not worry about that for now. If you have any time to make it possible to add bonds, then focus on that. Ignore the rest of this reply. If you have time later to make it possible to specify bond types, then I have a detailed procedure to make that possible which I explain below.

Suggestions for implementing manual bond typing:

The problems I see when bonds are (manually) typed are:

  • how can the user distinguish them? You kind of need to keep track of them, especially when scrolling through a trajectory where your changes may very well be local.

Visually, you mean? Hmm.. Good question! (Dammit.) I admit I did not think about that issue. I think I have an answer though. See below.

If it's possible to create custom bond types, should it be possible to choose different representations?

Ideally yes. But this would require yet another table (to store representations of different types of bonds.). Perhaps this is another reason why you should not worry about implementing custom bond types yet. (If you implement this, I suggest that the bond-types should be stored internally as strings, not integers. That's probably my most important suggestion. See below for details...)

  • Say we have a system with ca-ca, c3-c3 and ca-c3 bonds. c3-c3 would result in a type of 2 when exporting to LAMMPS. We change one of the c3-c3 bonds to have type 4. A few thought experiments:

(Great example, by the way...)

  • We change some other atom's type, so that we end up with a ca-c2 bond. What will be bond type 4?
  • We change one of our bond's atoms so we get c3-c2. Will this still be type 4?

In my mind, bond types should be internally represented in VIPSTER as strings, not integers. In this example, the bonds would have type names ca-ca, c3-c3 and ca-c3 and 4 (or ca&ca, c3&c3 and ca&c3 or something similar). Using strings instead of integers will open up many possibilities down the road, even if you don't have time to implement those possibilities now.

Notes:

bond representation

Presumably there would be a table somewhere storing the the representation for all of the bond types (eg. ca-ca, c3-c3 and ca-c3 and 4). The representations for the blank/automatic bond types (ca-ca, c3-c3 and ca-c3) would be automatically generated based on the atom type (as they are currently). And the manually entered bond types (eg 4) would be given a default representation (for example, a thin-grey line which is independent of the atom type). Eventually you could allow the user to edit this table (to change the appearance of this thin grey tube), but that's not a feature you would need to implement at the beginning.

Optional boring detail: a way to distinguish manual and auto/blank bond types

(Internally, you might want to use a weird character like '&' instead of '-', and make the '&' character illegal for the user to type into the 3rd column. That would gives us a way to distinguish bonds left with blank types which were automatically generated, like ca&ca, c3&c3 and ca&c3 from more legitimate sounding bond-names like ca-ca, c3-c3 and ca-c3 which the user could enter manually. Don't worry about this detail now.)

  • Will our custom type coincide by chance with future c3-c2 bonds or shall they be different?

They will be different. The user can customize the bond type between specific pairs of atoms (regardless of the atom type), but it should not effect other bonds between these two atom types. Since one of the bonds was manually assigned to type 4, that integer will be unavailable. All automatically generated (non-customized) bonds between these two atom types (which will be automatically assigned to the string c3-c2) will all be be assigned to the same integer type: whichever is not used up yet (probably 5 or 6, depending on the order of the list of bonds).

  • We change the type to 5. Do bond types in LAMMPS need to be consecutive?

Great question. Suppose one of the bond types was manually assigned to the string 10000 (for example), and suppose the user exports directly to LAMMPS data file format. LAMMPS will assume there are at least 10000 bond types and it will complain if the user neglects to specify bond parameters (eg bond-lengths and spring constants) for any of these 10000 bond types. However, in my opinion, this is not something that effects VIPSTER. VIPSTER should not worry about this issue. It is not an issue with the DATA file that VIPSTER would create because usually these bond force-field parameters do not (and should not) go in the DATA file. In my opinion (and Axel's opinion) bond parameters should go in a separate text file (an input script file) containing a list of "bond_coeff" statements (containing all of the bond-parameters).

If I understand correctly, creating lammps input scripts (with "bond_coeff" statements) is not VIPSTER's job. VIPSTER makes DATA files. (Incidentally, moltemplate makes input scripts.) This is squarely the user's job to find a way to supply LAMMPS with this file containing "bond_coeff" commands for bond types 1,2,3,4,5,...10000 (in this example). It is not possible for VIPSTER to worry about this detail.

If you want to be kind to the user, perhaps VIPSTER can print out a warning to the user whenever the resulting LAMMPS data file has nonconsecutive bond-type integers. (But it should be allowed. I can think of situations where the user might want to explicitly specify bond types explicitly in a way which is not guaranteed to be consecutive.) Again, great question.

  • We change the type to "test". Do we assign a different integer than for the regular atom pair when exporting to LAMMPS?

Again, you only need to represent this bond type with an integer if you are exporting to a file format (such as the LAMMPS data format) which requires integer bond types. As mentioned earlier, use one of the available integers which was not explicitly reserved by the user. If you are exporting to moltemplate format, you would leave this bond type as a string test.

  • We export to MOLTEMPLATE. When are integers valid strings?

Moltemplate represents all atom types, bond types (and even atom IDs and bond IDs) as strings. It does not care if that string is also a valid integer. Later on it converts all unique strings to unique integers. (And the user can customize how this is done.)

We add another file format plugin. How dedicated shall our mechanism be in determining whether the custom type fits the scheme? It boils down to this: If we have an optional widget with an optional feature to optionally set a bond's type, this shouldn't be able to result in an invalid file. And if it can be helped, also not in a wrong file.

All file formats I know of fall into two categories:

When exporting to file formats that use integers, you just have to be careful to check whether or not the integers begin at 0 or 1. (In LAMMPS, all atom IDs, atom types, and bond types must be assigned to positive integers starting at 1.)

In my biased opinion, moltemplate is pretty general. (Perhaps I am being egotistical when I say that.) If we (I) can get VIPSTER to export to moltemplate format, then I imagine that any other format that uses strings should be even easier to implement. In fact, I am happy to implement the code that exports VIPSTER to moltemplate format. But it makes it easier to generate meaningful moltemplate files if your VIPSTER code internally represent these bond and atom types as strings. That way I can use the same strings them when I export the molecules to moltemplate format. For now, you can go ahead and assign these strings automatically to ca-ca, c3-c3 and ca-c3 (or 1, 2, 3,...) We can worry about allowing users to be able to customize the bond type strings later (or never).

(Right now it would not effect the user at all. They would not even know if the bond types are implemented internally as integers or strings.)

Please don't take this too negatively! This list of problems is more for bookkeeping purposes than for shutting down the idea. I'll be thankful for all feedback.

I don't! It's good to discuss these things. We don't have to implement them. I just hope I'm not wearing you out.

jewettaij commented 5 years ago

My apologies. That last reply was pretty long. In Summary Don't worry about giving users the ability to customize bond types yet. However, if it's not too difficult I do suggest that you internally represent the bond-types as strings instead of integers. This will make the transition to more advanced bond-typing easier later on, should you ever choose to consider this (and it makes exporting to moltemplate a tiny bit easier too).

sgsaenger commented 5 years ago

This contained some pretty good ideas and insights! I think I've got some idea now how to go at this. An apparent issue regarding strings and speed is ofc the allocation. But this may be circumvented if the type is just stored as a pointer. nullptr will then fall back to automatic determination, and custom types will be added to a table akin to the periodic table, where the user may edit the properties, and the pointer will then point to the table entry. A similar approach I've planned for atom handling, where right now the name and type association are duplicated. If I can implement this for atoms, it should be straightforward to add this to bonds. Thanks again for the fruitful discussion!

sgsaenger commented 5 years ago

To give a small heads-up to the current state of this issue:

I've implemented:

Not implemented:

I've also enabled continous delivery, so if you're on linux, feel free to try out the current state and tell me what you think: https://github.com/sgsaenger/vipster/releases/tag/continuous

sgsaenger commented 5 years ago

Drawing and editing bonds with the mouse are now implemented, too.

I think this issue should be mostly solved for now, but I leave this decision to you. What I see missing now is mostly two things, namely handling bonds to deleted atoms, which I'd like to postpone until I can rework general tracking of changes in the library, unless it comes up as a common problem; in any case, i see it as a different issue. The other thing is improving the usability. There's relatively little feedback when using both the bond mousemode or the list of bonds. It could be nice to e.g. highlight atoms involved in a certain bond, but I'm not sure if it's not overly irritating when the bond-tooling overrides the atom-selection.

I'd be happy for some feedback, especially regarding #32: do you find the implementation as-is intuitive, and do you think it covers your use-cases?

The continuous build is available also for other OS's, btw.

jewettaij commented 5 years ago

Hi Sebastian I tried out the new build, and I think it is very promising and exciting! I was able to get all of these new features to work, which is great (except changing the color, but I don't care so much about that). I also noticed tooltips (?) are now working! Even the list of bond type strings is included the in the final data file, which is great.

minor request

The ? tooltip next to the "Bonds" button says:

"In manual mode the user can add/delete bonds one by one, but also request generating bonds by the same criteria as in automatic mode."

(I'm not quite sure what you meant by "but also request generating bonds by the same criteria as in automatic mode.")

Can we change it to say: "In manual mode the user can add/delete bonds one by one by clicking on them with the middle mouse button."

(If you want to add the rest of the sentence ("but also request generating bonds..."), that's fine too. I just wanted to make it clear that users should use the middle mouse button.)

Perhaps you could also say: "You must click the "(Re-)calculate bonds" button after deleting atoms. You may also need to move the atoms further apart to clear the bonds, even in manual mode." This procedure seems to get rid of the bonds and angles, etc...

Progress from my end:

vipster-->moltemplate conversion

Selfishly speaking, this is something that probably is more useful to me than it is to you. I wanted a graphical way for users to create moltempate molecule files. Thanks to you, I think we are very, very close to this.

I modified the "ltemplify.py" data_file->moltemplate converter to pay attention to atom types in the comments of the "Masses" section. Now I just need to modify it to pay attention to the bond type strings (and angle type strings, etc...) Then it will generate moltemplate files with meaningful atom and bond type names.

This means that we don't have to edit the VIPSTER code to output to moltemplate format. Just tell VIPSTER to invoke "ltemplify.py -useff NAME_OF_DATA_FILE", and it will generate a file that moltemplate can read.

However there is one small change that I still need to make to "ltemplify.py": I have not added the "-useff" argument yet (so the command above won't work yet). I will get that working over the weekend and contact you again.

Excellent!!

-Andrew I'm happy to close this issue, but I can also wait until you are satisfied with how VIPSTER responds to deleted atoms.

sgsaenger commented 5 years ago

I was able to get all of these new features to work, which is great (except changing the color, but I don't care so much about that).

Could you elaborate on this? Colors should be working just fine. Maybe it's a misunderstanding because only custom bond types can be colored?

"In manual mode the user can add/delete bonds one by one, but also request generating bonds by the same criteria as in automatic mode."

(I'm not quite sure what you meant by "but also request generating bonds by the same criteria as in automatic mode.")

This refers to the "regenerate bonds" button, which will execute the regular bond-generation algorithm. I guess I should be more explicit here. This button would also clear custom bond types, so a warning should also be issued...

Can we change it to say: "In manual mode the user can add/delete bonds one by one by clicking on them with the middle mouse button."

(If you want to add the rest of the sentence ("but also request generating bonds..."), that's fine too. I just wanted to make it clear that users should use the middle mouse button.)

In principle, all mouse buttons should work. Did you have problems using left/right click or just assume middle click? Also, clicking on the bond should do nothing. Only connecting atoms should trigger a bond toggle. Did you have a different experience?

Perhaps you could also say: "You must click the "(Re-)calculate bonds" button after deleting atoms. You may also need to move the atoms further apart to clear the bonds, even in manual mode." This procedure seems to get rid of the bonds and angles, etc...

I'm not sure how to proceed with deleted atoms in the short term. As mentioned, I have a plan to rework handling changes to the molecule completely, which would tie in with this problem. Recommending the recalc button does not feel like a solution however, as this would reset all bonds, and erase their custom types. Why would the user need to move atoms in manual mode? Distances are irrelevant here. Or do you intend to use the recalc button to clear all bonds? In this case it would be easier to add a button to clear bonds, than to instruct the user to abuse the mechanism. Is this something you'd expect/recommend to implement?

Progress from my end:

vipster-->moltemplate conversion

[...]

However there is one small change that I still need to make to "ltemplify.py": I have not added the "-useff" argument yet (so the command above won't work yet). I will get that working over the weekend and contact you again.

Now that continuous builds are working, I'm going to have to merge the branch with embedded python functionality. I'll get to you when this works, this should make interaction with moltemplate much easier.

I'm happy to close this issue, but I can also wait until you are satisfied with how VIPSTER responds to deleted atoms.

Let's make sure we're on the same page regarding these features first. It's hard to implement features when you're not actively using them, so if I've made some assumptions that go against your intuition the issue still stands.

jewettaij commented 5 years ago

I'm sorry that some of the things I mentioned in my previous post were confusing or wrong. I don't have time to address them tonight, but I'll definitely reply to your questions tomorrow.

I have just very short message for now:

I'm excited to say that I just implemented the changes to "ltemplify.py" that are needed to automatically convert VIPSTER output into moltemplate (.LT) files. This means that we could easily add moltemplate export capability to VIPSTER. (...and this would make me very happy...) Currently "ltemplify.py" is a stand-alone python script that was intended to be run from the shell. Syntax:

ltemplify.py DATA_FILE > MOLTEMPLATE_FILE

(This syntax probably only works in a terminal that understands what the ">" redirect symbol means. I'm not sure if WINDOWS/DOS supports this.) In python you can invoke the shell using the "os.system()" call. I'm not sure if that is the way you want to do it. If necessary, I could change "ltemplify.py" to make it possible to run ltemplify.py entirely from within python (ie. without needing the shell). But in that case I have a question for you. (I'll hold off on asking it now.)

Thank you for your patience. I'll explain this more, and I'll get to your other questions tomorrow.

Thanks for your hard work on this feature! -Andrew

jewettaij commented 5 years ago

I was able to get all of these new features to work, which is great (except changing the color, but I don't care so much about that).

Could you elaborate on this?

Sure! When I have created a bond manually, nothing happens if I click on the "Color" box in the table (the white box in the "Color" column). The color of the box is white, but the bond itself is a combination of the colors of the two atoms it is connected to (eg, black, green, grey, ...). I am not bothered much by the inability to customize bond color. If it's not possible to change the bond color, that's okay. But in that case, would it be better to remove the "Color" column from the table?

Colors should be working just fine. Maybe it's a misunderstanding because only custom bond types can be colored?

I must be doing something wrong then.

"In manual mode the user can add/delete bonds one by one, but also request generating bonds by the same criteria as in automatic mode." (I'm not quite sure what you meant by "but also request generating bonds by the same criteria as in automatic mode.")

This refers to the "regenerate bonds" button, which will execute the regular bond-generation algorithm. I guess I should be more explicit here. This button would also clear custom bond types, so a warning should also be issued...

Okay. That would help.

Can we change it to say: "In manual mode the user can add/delete bonds one by one by clicking on them with the middle mouse button." (If you want to add the rest of the sentence ("but also request generating bonds..."), that's fine too. I just wanted to make it clear that users should use the middle mouse button.)

In principle, all mouse buttons should work. Did you have problems using left/right click or just assume middle click?

Oops. Sorry. I just assumed it only works with the middle mouse button. I tried it with other mouse buttons and it's working. Please disregard this suggestion.

Also, clicking on the bond should do nothing. Only connecting atoms should trigger a bond toggle. Did you have a different experience?

Nope. Once I tried clicking on atoms, everything behaved as expected. Please disregard my comment. Everything is fine.

Perhaps you could also say: "You must click the "(Re-)calculate bonds" button after deleting atoms. You may also need to move the atoms further apart to clear the bonds, even in manual mode." This procedure seems to get rid of the bonds and angles, etc...

I'm not sure how to proceed with deleted atoms in the short term. As mentioned, I have a plan to rework handling changes to the molecule completely, which would tie in with this problem. Recommending the recalc button does not feel like a solution however, as this would reset all bonds, and erase their custom types.

I see. Please disregard my suggestion here too in that case.

Why would the user need to move atoms in manual mode? Distances are irrelevant here. Or do you intend to use the recalc button to clear all bonds? In this case it would be easier to add a button to clear bonds, than to instruct the user to abuse the mechanism. Is this something you'd expect/recommend to implement?

You are correct. This suggestion of mine did not make sense. Again, please disregard it.

Progress from my end:

vipster-->moltemplate conversion

[...] However there is one small change that I still need to make to "ltemplify.py": I have not added the "-useff" argument yet (so the command above won't work yet). I will get that working over the weekend and contact you again.

Now that continuous builds are working, I'm going to have to merge the branch with embedded python functionality. I'll get to you when this works, this should make interaction with moltemplate much easier.

I'm happy to close this issue, but I can also wait until you are satisfied with how VIPSTER responds to deleted atoms.

Let's make sure we're on the same page regarding these features first. It's hard to implement features when you're not actively using them, so if I've made some assumptions that go against your intuition the issue still stands.

Okay. I'll wait until you figure out a way to delete atoms. It's probably important to get that working. After that, feel free to close this issue.

Cheers I'm about to post another issue requesting moltemplate compatibility. It will go further into the details how to use the python script I wrote ("ltemplify.py") to convert VIPSTER output into moltemplate format.

jewettaij commented 5 years ago

Colors should be working just fine. Maybe it's a misunderstanding because only custom bond types can be colored?

I must be doing something wrong then.

I'm sorry. I just read your reply more carefully and tried using VIPSTER again. It is indeed possible to change the color of a bond I created manually. I must have been trying to do this with bonds that were created automatically.

small feature request

In that case, would it be difficult to remove or hide the "Color" button during "Automatic" bond creation? Alternatively, perhaps you could put something in the Color box to warn users that they cannot change the color (like "N/A", or a grey pattern).

Cheers -Andrew

sgsaenger commented 5 years ago

Thanks for your clarifications, I've implemented your feedback.