-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Interactions for MD trajectories #719
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! I've added some comments for cleanup and simplification of code.
molecularnodes/handlers.py
Outdated
if entity._entity_type.value == "interaction": | ||
print("An entity has been found") | ||
entity.update_bond_positions(scene.frame_current) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the MolecularEntity
class has a set_frame()
method. If you override it (with the contents of update_bond_positions) for the interaction class, then in the already existing update_trajectories()
method above, can just loop through all entities, and call the set_frame
method instead of creating a whole new per-frame update method.
The plan is the make the update_trajectories
function more generic, just calling it update_entities
so that it can potentially update any kind of entity, but that will be done in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that confused me a little bit as you indicated that all the subclasses that had for super class MolecularEntity
would be updated by the update_trajectories
function thanks to the set_frame()
method. From what I understood only the trajectories are currently affected because update_trajectories
only loops over the trajectories and not all the entities. For that I added the update_interaction
function. Maybe I've missed something, though !
I also think going for this more generic update_entities
function is the way to go. Should I keep the current code waiting for the PR or should I modify it having this PR in mind ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put together the changes for making it more general in #721 , so once that is merged you can bring it into this branch and use the handler there. Sorry for the initial confusion around it, I thought I had already gone back and generalised it!
I additionally moved the text from your most recent comment on issue #692 into the PR description, so that it's easier to discuss. I'd like to bring in @yuxuanzhuang and @PardhavMaradani to give feedback on naming. It might make more sense to name this new entity |
Additionally could you please add some example data (an example trajectory and |
Of course ! Currently the demo data is stored in my old extension repo Viber. In the I don't know if you want me to send you those files elsewhere, please tell me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still learning how to handle pull requests, I hope it is not messy.
if visible: | ||
spline = obj.data.splines[0] | ||
|
||
if interaction_type == "PiStacking": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider this section incomplete but functional as is. I would like to complexify the way interactions are represented, having a different treatment for the different interactions.
For example the pi-stacking is currently represented by a bond that links two centroids, one for all the concerned atoms in the protein and one for those in the ligand. If I take the example of a pi-cation interaction, the same treatment should be applied but is currently not implement. In the demo files I will give you (in the relevant comment), you can see a pi-cation interaction that is still not treated with a bond being created between each aromatic atom and the cation (around the frame 80 if I remember correctly, I will have to check that).
Anyways, to the different interaction existing and the numerous ways to represent them, what would be your opinion on the implementation ? Providing these interaction utilities in an already complex state, or releasing it in a minimal and working state (close to the current one so hbond, pi-stacking and salt bridge) that will be improved later on ?
I think that because everybody can have preferences of representation, it would be more interesting to make it minimal and have the community involved to then pick the most preferred or common representation. You may also have personal preferences which should be applied as you own this project !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that minimal is the way to go. The overall ethos of Molecular Nodes is more to give as much power & access to the underlying data and visualisations to the user as possible. I think starting with this and then potentially making it more involved in the future is the way to go.
molecularnodes/handlers.py
Outdated
if entity._entity_type.value == "interaction": | ||
print("An entity has been found") | ||
entity.update_bond_positions(scene.frame_current) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that confused me a little bit as you indicated that all the subclasses that had for super class MolecularEntity
would be updated by the update_trajectories
function thanks to the set_frame()
method. From what I understood only the trajectories are currently affected because update_trajectories
only loops over the trajectories and not all the entities. For that I added the update_interaction
function. Maybe I've missed something, though !
I also think going for this more generic update_entities
function is the way to go. Should I keep the current code waiting for the PR or should I modify it having this PR in mind ?
Hi Brady, the code in |
I've just done some testing - on Blender 4.3 it fails when attempting to add the interaction: Error: Python: Traceback (most recent call last):
File "/Users/brady/Library/Application Support/Blender/4.3/extensions/vscode_development/molecularnodes/entities/interaction/interaction.py", line 242, in execute
interaction.setup_from_json(json_file, object_name)
File "/Users/brady/Library/Application Support/Blender/4.3/extensions/vscode_development/molecularnodes/entities/interaction/interaction.py", line 202, in setup_from_json
bond_material = self.create_bond_material(interaction_type)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/brady/Library/Application Support/Blender/4.3/extensions/vscode_development/molecularnodes/entities/interaction/interaction.py", line 97, in create_bond_material
mat.shadow_method = "CLIP"
^^^^^^^^^^^^^^^^^
AttributeError: 'Material' object has no attribute 'shadow_method' This is something that will need fixing, the material API may have changed a bit going into 4.3. |
@MartinBaGar was closing of this PR intentional? |
Not at all ! I might have gotten confused in the process, I am sorry. For now I successfully added the interactions options in the molecular nodes panel so it automatically works with the active object. I also modified the interaction class so it uses the |
No worries at all! It can be be a lot of things to try and keep track of when learning PRs & GitHub. |
If you have questions around |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #719 +/- ##
==========================================
- Coverage 79.06% 74.82% -4.25%
==========================================
Files 80 82 +2
Lines 5317 5478 +161
==========================================
- Hits 4204 4099 -105
- Misses 1113 1379 +266 ☔ View full report in Codecov by Sentry. |
Thanks for your comprehension ! For info, the api I used was indeed incompatible with 4.3, I found the info here if you are interested. So now I have the main branch of my MolecularNodes fork that works with the implementation in the Molecular Nodes panel. I will definitely check PS: My current branch doesn't pass two codecov checks I don't know why. I hope it is not a problem for the future. |
The code-cov checks don't worry about for now. They are to check that tests are covering all of the new code. Before the final merge we'll want to ensure that there are some new tests written that covert the new functionality, but for now we can ignore them. I would recommend focusing first changing the interface object to a single mesh object first. The save / reload system should in theory work 'out of the box' with a mesh object, so I would recommend porting that over first then we can make sure that the object saves & reloads as expected. |
Ok thank you for the info on code-cov ! I am not sure to understand what you say about changing the interface object to a single mesh object. Currently the Please tell me if I'm not clear. |
Yes this is what I am meaning. With all data contained in a single object / mesh, it'll be easier to handler overall. For each interaction type and frame, we can just store an attribute on the mesh which will let us use different materials later. import databpy as db
bob = db.create_bob(positions_array edges_array)
bob.store_named_attribute(interaction_type_number, "interaction_type")
bob.store_named_attribute(interaction_colors, "Color") We could store attributes for the interaction type, and we can then use that value in the shader to set colors (or store our own color attribute). We can also use those values to change other parts of the interaction. The way I am currently thinking, is that the def set_frame(self, frame: int):
bob = BlenderObject(self.object)
bob.new_from_pydata(current_interaction_positions, current_interaction_edges)
bob.store_named_attribute(current_interaction_types, "interaction_type") This way we are clearing the data and remaking the edges and vertices for each frame, which can also then take into account interpolation and averaging that the trajectories themselves support. |
Ok very cool ! Thank you for the clarification. This seems indeed to be a much more flexible solution. I will refactor my code in this way. |
did you intend to close this again @MartinBaGar ? |
Ah again some git issues, I am really sorry for that. However I have some better news ! I refactored my code to create the interaction object using databpy. I have also changed the way bonds are created so now it is done with geometry nodes which I think is more interesting for customisation (like setting the bond width which was previously managed by an operator). And of course the loading of the interaction file is accessible in the MN panel. And now with your recent modification, specially the handler, it is updated frame by frame and operating when reloading the session. I have pushed my last modifications so it should be useable as is. Please tell me if it is going in the right direction ! Thanks again for the time reviewing this ! |
Cleans up and minimises code use. As the Interaction is a subclass of `MolecularEntity`, we can use the self.store_named_attribute() methods, and reference the object via self.object.
Hi @MartinBaGar unsure if you got the notification but in case you didn't, I review and changed some stuff but opened it up as a separate PR into your PR so that I could explain some of the changes. If you accept that one (which can just be done through the Blender UI) then those changes should be reflected here. |
Hi @BradyAJohnston ! Indeed I did not see the notification. I will accept it as soon as i can today. |
A bit of cleanup and using self.*_named_attribute
Back from the other PR. The JSON file structure we're using now is a bit outdated with the new way the interaction objects are created. To keep things compatible, I had to reorganize it using the I already have a notebook that shows what this new structure should look like and how to generate it. I'll update it with the new structure and turn it into a small Quarto page for documentation. That way, it'll be easy for everyone to understand and use. I've been using ProLIF to generate demo data, and it got me thinking – why not implement most of the interactions it handles? It could be reduced to : Let me know what you think! Any suggestions or ideas are super welcome. |
I think generalising to more interactions is the right approach, so if you want to refactor a bit to better support it that sounds like a good plan! |
As a follow up to the #692 issue. Please tell me if you want any additional information to the previous issue.
Looking forward to your comments !
Functionalities
universe
.Things that could be added
The implementation
The main part of the interaction visualiser was implmented in
molecularnodes/entities/interaction
.The initial script was split into two parts, the ui, under
ui.py
, and the operations, underinteraction.py
.As in the other folders,
entities/interaction/__init__.py
collects all theCLASSES
in its folder.An
EntityType
was added:In
entities/__init__.py
the new interaction classes are imported fromentities/interaction/__init__.py
.A pointer to the interaction visualiser properties was added to
addon.py
.Also, to have the handler perform per-frame-updates, a new function
update_interactions
was added tohandlers.py
to work on all the entity objects havingEntityType.INTERACTION
. This was added as aframe_change_post
handler which is registered and unregistered likeframe_change_pre
in theaddon.py
file. Since the interaction objects position point towards the trajectory entity, I think it is better to use aframe_change_post
handler.addon.py
Current issues
The bond objects are not updated when the blender session is reopened, for now the alternative is to delete the interaction hierarchy (right-click on the interaction collection in the scene panel > delete hierarchy) and click again on "Visualise Interactions". It might be due to a session error.
Show case
Some pics to show different bond width.