Molecule class implementation

[rbdavid]

Here's some notes that I have for the current implementation of the Molecule class:

1. I'm noticing some methods associated with the Molecule superclass that utilize self.data (see below), which is not an instantiated class variable in either the Molecule class nor the PDB, CIF, MMTF subclasses.

https://github.com/rbdavid/MolecularNodes/blob/8f70f3195041b15b65da97ff60f1fa17258712d4/molecularnodes/io/parse/molecule.py#L337-L365

The __len(self)__ method uses self.object.data.vertices. This would be pulling the bpy.Object's data vertices values and taking the length. Maybe all of the self.data instances should be using self.object.data as well?

Additionally, the __getitem__ method uses get_attribute(index), which is wrongly implemented for the Molecule.get_attribute() defined on https://github.com/rbdavid/MolecularNodes/blob/8f70f3195041b15b65da97ff60f1fa17258712d4/molecularnodes/io/parse/molecule.py#L110-L133. Maybe this function was intended to get one atom's values for all attributes?

2. Also, as noted in Centre upon import by either CoG or CoM #448 (comment), there are instances of bpy.types.Object.mn() lines that cause errors when I test the Molecule creation process on my local desktop.

3. The Molecule.get_attribute() and similar set_attribute() have the same name space as the functions defined in molecularnodes/blender/obj.py. The Molecule methods are just wrappers for those functions. This was actually quite confusing for me during coding up CoM/CoG methods because the Molecule methods will fail if called before setting self.object = model within the `Molecule.create_model() method. I'm not sure if the Molecule methods are actually necessary since they are just wrapper functions for functions that act on a different object class all-together.

4. This might be a variable naming issue but frames in the Molecule.create_model() method is actually a bpy.types.Collection object rather than a list/iteratable of bpy.types.Objects. So maybe it should either be called frame_collection or be changed to a list of the blender objects within that collection.

[BradyAJohnston]

This might be a variable naming issue but frames in the Molecule.create_model() method is actually a bpy.types.Collection object rather than a list/iteratable of bpy.types.Objects. So maybe it should either be called frame_collection or be changed to a list of the blender objects within that collection.

I think you are right, that we should be calling it frames_collection to be explicit that we aren't dealing with frames inside of Blender, but a collection of 'frame' objects.

The Molecule.get_attribute() and similar set_attribute() have the same name space as the functions defined in molecularnodes/blender/obj.py.

This is part of a broader discussion around how best to design the API for interacting with Blender objects. My thought process in implementing it this way, was to have the blender submodule be a generic and cleaner way to interact with the objects and API in blender. That way it can be used more generically across the add-on and even potentially in other add-ons.

The idea behind wrapping them for the classes is for convenience and cleaned code, that calls the underlying generic way of interacting with Blender's API. It made sense to me when implementing it, but I am open to suggestions for changing it if you think it could be improved.

mol.get_attribute('position')
# vs
mn.blender.obj.get_attribute(mol, 'position')

There is also a separate discussion around naming for get_attribute() and set_attribute(), as these are already ways for gettting / setting OOP attributes in python.

It was the best I could think of at the time, but I might switch them to named_attribute() and store_named_attribute() to align better with their 'node equivalents'.

I'm noticing some methods associated with the Molecule superclass that utilize self.data (see below), which is not an instantiated class variable in either the Molecule class nor the PDB, CIF, MMTF subclasses.

Some of these are leftovers of GitHub Copilot generated boilerplate that I didn't get around to cleaning up if I am being honest. I haven't really implemented anything that uses them - so it was on the todo list to clean up.

You raise an important point that about how these methods should work (or if they are relevant) which also touches on discussions in other threads.

Currently the Molecule object (and others) have the self.data and self.object. When using named_attribute() or store_named_attribute() we are getting and setting on the Blender object - should we be updating the underlying data biotite.AtomArray or MDAnalysis.Universe? Tracking and having two sources of 'truth' I am not a big fan of.

My thoughts are that once we have everything inside of Blender we should pretty much just be interacting with the object, and we should just make sure we have pulled all relevant information from the file itself, with the option to 're-parse' the file for additional information if required for specific use cases.

This is all very relevant to #359 so I am tagging it for cross-referencing

[BradyAJohnston]

This was actually quite confusing for me during coding up CoM/CoG methods because the Molecule methods will fail if called before setting self.object = model within the `Molecule.create_model() method. I'm not sure if the Molecule methods are actually necessary since they are just wrapper functions for functions that act on a different object class all-together.

I'm much more familiar with a functional style of programming - but I have tried to change the underlying workflow to utilise classes (this project is my first time doing so - so I welcome any and all suggestions for improving the implementations).

I was unsure what to leave as a 'generic' function that could be used in multiple places and what should be class-associated methods. Certainly looking at it now, methods like create_model() should maybe just be the full method rather than calling the function _create_model().

[rbdavid]

The idea behind wrapping them for the classes is for convenience and cleaned code, that calls the underlying generic way of interacting with Blender's API. It made sense to me when implementing it, but I am open to suggestions for changing it if you think it could be improved.

mol.get_attribute('position')
# vs
mn.blender.obj.get_attribute(mol, 'position')

There isn't much gained by the wrappers, especially when thinking about maintaining simple code. The code outline/flow currently stands as the Molecule.create_model() calls the Molecule.get_attribute() that then calls the mn.blender.obj.get_attribute(). Three jumps instead of two to figure out what the get_attribute() is really doing.

These wrapper functions all return the same warning when the methods are called before the self.object = model line is run. I feel like that line should be immediately after the _create_model() call? Or, is it even necessary to assign the bpy.types.Object to a Molecule class variable when the whole Molecule.create_model() method returns the model?

There is also a separate discussion around naming for get_attribute() and set_attribute(), as these are already ways for gettting / setting OOP attributes in python.

It was the best I could think of at the time, but I might switch them to named_attribute() and store_named_attribute() to align better with their 'node equivalents'.

Those names seem as good as any others.

[rbdavid]

Currently the Molecule object (and others) have the self.data and self.object. When using named_attribute() or store_named_attribute() we are getting and setting on the Blender object - should we be updating the underlying data biotite.AtomArray or MDAnalysis.Universe? Tracking and having two sources of 'truth' I am not a big fan of.

My thoughts are that once we have everything inside of Blender we should pretty much just be interacting with the object, and we should just make sure we have pulled all relevant information from the file itself, with the option to 're-parse' the file for additional information if required for specific use cases.

This is all very relevant to #359 so I am tagging it for cross-referencing

Yeah, the idea of the data twin to the blender object is maybe something that I implement outside of MN. Implementing the twin within the MN code so that 'truth' is consistent is a heavy lift for a generalizable addon. Conversely, recovering the abilities that pymol or VMD have require the use of structural biology libraries; its certainly not reasonable to expect MN to do those things natively.

I actually don't think the "updating the truth" of two objects to be consistent is too hard. All you have to do is worry about coordinates and frame numbers (if a multiframe model). I don't think I'll need to worry about alterations made for keyframing the blender object's location/rotation/scaling; those are local changes that don't affect the vertex attributes. AFAIK, you can't keyframe changes to the internal vertex attributes.

[rbdavid]

I'm much more familiar with a functional style of programming - but I have tried to change the underlying workflow to utilise classes (this project is my first time doing so - so I welcome any and all suggestions for improving the implementations).

You're miles past me. I haven't messed around with Class objects until now. I think the Molecule class and its subclasses are the correct design for what they're doing. And I'm happy to help implement changes, if you'd like.

I was unsure what to leave as a 'generic' function that could be used in multiple places and what should be class-associated methods. Certainly looking at it now, methods like create_model() should maybe just be the full method rather than calling the function _create_model().

Yeah, that's true; the attribute functions could still be used within the method and all that. In a similar vein, could the fetch() and load() functions for wwpdb.py and local.py be implemented within the respective MN_OT_Import classes? Idk, that might be unnecessary. Why fix something that isn't broke?

[BradyAJohnston]

Yeah, that's true; the attribute functions could still be used within the method and all that. In a similar vein, could the fetch() and load() functions for wwpdb.py and local.py be implemented within the respective MN_OT_Import classes? Idk, that might be unnecessary. Why fix something that isn't broke?

Operators in Blender aren't the best to work with via scripting, especially in a jupyter notebook or script. Because of this, I've designed it such that each import method is handled by one overarching function. This function can be called reliably from a script, and the operator that is executed when pressing a button just runs that one function with the necessary parameters. It adds an extra layer of abstraction, but I believe is necessary to have the operator and scripting interfaces be on par with each other.