should adding an existing ToplogyAttr raise an exception or at least keep existing values?

[Oliver Beckstein]

Hi devs,

Just came across the following that throws off users:

When I add a TopologyAttr that already exists, it will silently delete all existing data. For example, names exists but when I add it again, all the names are gone:

>>> u1 = mda.Universe.from_smiles("CCO")
>>> print(u1.atoms.names)
['C0' 'C1' 'O2' 'H3' 'H4' 'H5' 'H6' 'H7' 'H8']
>>> u1.add_TopologyAttr("names")
>>> print(u1.atoms.names)

['' '' '' '' '' '' '' '' ‘']

I see different options for handling this:

1) Keep current behavior. The expectation is that anyone using add_TopolAttr() knows what they are doing….  One can also make a case that the current behavior is similar to Python just allowing you change any attribute of an object via assignment without ever checking that this makes sense. 

2) Raise an exception. Perhaps AttributeError???

3) Make it a pass if the attribute already exists — arguably, the object already provides what the user wants and it even has better initial values than all blanks.

I think my preferred solution is (3) because it seems weird to use add_TopologyAttr() with the intent to delete all existing values.

Any opinions?

Oliver

--
Oliver Beckstein, DPhil * oliver.b...@asu.edu
https://becksteinlab.physics.asu.edu/

pronouns: he/his/him

Associate Professor of Physics
Arizona State University
Center for Biological Physics and Department of Physics
Tempe, AZ 85287-1504
USA

[ialibay]

I'm not sure I fully understand your option 3 here, are you proposing that using add_TopologyAttr only updates blank values?

Personally I'm somewhere between option 1 or 2.

Option 1 is more representative of a standard Python dictionary, it is known behaviour and in many ways that's fine.

Option 2 makes sense when you consider the name of the method, it makes no sense for you to "add an attribute" if it already exists.

My take would be to go for a slight middle ground of `add_TopologyAttr(topologyattr, overwrite=False)` where by default it raises an AttributeError if the attribute already exists, and you can allow it to be overriden if you set overwrite=True. This would also allow us to set the default to overwrite=False until we reach the v3.0 release of MDAnalysis.

- Irfan

[Oliver Beckstein]

On Feb 11, 2023, at 8:29 PM, ialibay <ial...@mdanalysis.org> wrote:

I'm not sure I fully understand your option 3 here, are you proposing that using add_TopologyAttr only updates blank values?

No, what I mean was the following:

For u.add_TopologyAttr(’name’)

* If the topology already has the attribute ’name’ then do nothing.

* If the topology does not have the attribute then add it with the default (empty strings in this case)

For u.add_TopologyAttr(’name’, values)

* Always set the ’name’ attribute to the explicitly given values. (If it doesn’t exist, create it.)

In pseudocode:

def add_TopologyAttr(u, identifier, values=None):

   if not has_TopolAttr(u, identifier):

       create_TopolAttr(u, identifier)

       set_TopolAttr(u, values)     # values=None will use the default for the TopolAttr

   else:

       if values is not None:

           set_TopolAttr(u, values)

Personally I'm somewhere between option 1 or 2.

Option 1 is more representative of a standard Python dictionary, it is known behaviour and in many ways that's fine.

True, although the way we work with TopologyAttrs does not look like using a dict. To me it feels more like “attaching something new and enhancing functionality”.

If anything, dict.setdefault(key[,value]) seems closer in spirit, I think.

Option 2 makes sense when you consider the name of the method, it makes no sense for you to "add an attribute" if it already exists.

My take would be to go for a slight middle ground of `add_TopologyAttr(topologyattr, overwrite=False)` where by default it raises an AttributeError if the attribute already exists, and you can allow it to be overriden if you set overwrite=True. This would also allow us to set the default to overwrite=False until we reach the v3.0 release of MDAnalysis.

That’s a possibility even though I’d prefer not adding another kwarg. However, doing anything here would likely count as an API change so we might not have a lot of choice.

We can always not do anything. But I thought I’d ask if people would want to consider changing it. Because then we could make a GSOC-starter issue out of it...

Oliver

-- 
You received this message because you are subscribed to the Google Groups "MDnalysis-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mdnalysis-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/mdnalysis-devel/9a131ee2-30d6-4a15-beee-16bf33d901d7n%40googlegroups.com.

--

Oliver Beckstein (he/his/him)

orbe...@gmail.com

[Lily Wang]

I like Oliver’s suggestion, although I’d suggest having at least a warning if the attribute already exists.

Cheers,

Lily

To view this discussion on the web visit https://groups.google.com/d/msgid/mdnalysis-devel/F7BF2798-AB8A-4E4F-B426-ED3FC11FAC5C%40gmail.com.