Skip to content
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

ignore minor error when initialising selection #745

Merged
merged 4 commits into from
Feb 23, 2025

Conversation

BradyAJohnston
Copy link
Owner

I can't currenlty figure out a nice way to have selections update when their UIList properties are updated, but not throw errors when starting, so for now we can just ignore them if they aren't present.

@BradyAJohnston
Copy link
Owner Author

@PardhavMaradani I can't seem to assign you as a reviewer but if you wanted to check it out that would be great

@PardhavMaradani
Copy link
Contributor

The changes looks fine to me.

This is how I had to break it apart to get past this error. Just fyi - in case you think it is any better.

diff --git a/molecularnodes/entities/trajectory/base.py b/molecularnodes/entities/trajectory/base.py
index 652fd52..165fa87 100644
--- a/molecularnodes/entities/trajectory/base.py
+++ b/molecularnodes/entities/trajectory/base.py
@@ -49,14 +49,13 @@ class Trajectory(MolecularEntity):
         periodic: bool = True,
     ):
         "Adds a new selection with the given name, selection string and selection parameters."
-        selection = Selection(
-            trajectory=self,
+        selection = Selection(trajectory=self, name=name)
+        self.selections[selection.name] = selection
+        selection.add_selection_property(
             selection_str=selection_str,
-            name=name,
             updating=updating,
             periodic=periodic,
         )
-        self.selections[selection.name] = selection
 
         return selection
 
diff --git a/molecularnodes/entities/trajectory/selections.py b/molecularnodes/entities/trajectory/selections.py
index dd56820..0f9ddc8 100644
--- a/molecularnodes/entities/trajectory/selections.py
+++ b/molecularnodes/entities/trajectory/selections.py
@@ -4,14 +4,14 @@ import numpy as np
 from uuid import uuid1
 
 class Selection:
-    def __init__(
-        self, trajectory, name: str = "selection_0", selection_str: str = "all",  updating=True, periodic=True
-    ):  
+    def __init__(self, trajectory, name: str = "selection_0"):
         self._ag: mda.AtomGroup | None = None
         self._uuid: str = str(uuid1())
         self._name = name
         self._current_selection_str: str = ""
         self.trajectory = trajectory
+
+    def add_selection_property(self, selection_str: str = "all", updating=True, periodic=True):
         self.trajectory.object.mn_trajectory_selections.add()
         self.trajectory.object.mn_trajectory_selections[-1].name = self.name
         self.trajectory.object.mn_trajectory_selections[-1].uuid = self._uuid

@BradyAJohnston
Copy link
Owner Author

I've been trying to keep the creation of the UIList property tightly coupled to the creation of the Selection, but maybe going with your method and decoupling then is a better strategy. Might reduce headaches overall.

@PardhavMaradani
Copy link
Contributor

As the add() method of a CollectionProperty returns the property added, it could be simplified to something like:

def add_selection_property(self, selection_str: str = "all", updating=True, periodic=True):
    prop = self.trajectory.object.mn_trajectory_selections.add()
    prop.name = self.name
    prop.uuid = self._uuid
    .....

as well.

Copy link

codecov bot commented Feb 23, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.06%. Comparing base (9d371e1) to head (95694b9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
molecularnodes/entities/trajectory/selections.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #745      +/-   ##
==========================================
- Coverage   79.06%   79.06%   -0.01%     
==========================================
  Files          80       80              
  Lines        5317     5320       +3     
==========================================
+ Hits         4204     4206       +2     
- Misses       1113     1114       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BradyAJohnston BradyAJohnston merged commit c272216 into main Feb 23, 2025
17 checks passed
@BradyAJohnston BradyAJohnston deleted the 744-selection-error branch February 23, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a new selection (both from UI and API) throws error, but selection is created
2 participants