Names not preserved in Geometry.rotate

[pfebrer]

I had some geometry with asigned names (groups of atoms). I wanted to apply several rotations applying this names, but after the first one the names were lost and therefore it didn't recognize what I wanted to rotate.

My workaround was to set the names after each rotation, but I don't think this should be needed.

This might be an issue for other methods that return new geometries (not copied ones), I haven't checked.

One thing to quickly solve the issue on multiple methods in an explicit way could be to build a wrapper, like so:

def preserve(*attrs, force=False):
    '''
    Wrapper constructor that makes sure *attrs are preserved on new objects
    '''
    
    def wrapper(method):
        
        def preserving_method(self, *args, **kwargs):
            
            # Run the actual method
            obj = method(self, *args, **kwargs)
            
            # Ensure the attributes are there, if not copy them from the instance 
            for attr in attrs:
                if force or not hasattr(obj, attr):
                    try:
                        val = getattr(self, attr)
                        setattr(obj, attr, val)
                    except AttributeError:
                        pass
                        
            return obj
        
        return preserving_method
    
    return wrapper

Then method definition could be like:

class Geometry(...):

    @preserve("names")
    def rotate(self, ...):
           # ...do things
           return geom # The wrapper would ensure that this geometry has the "names" attribute

This wrapper could work for other objects too, not only geometries.

What do you think? :)

[zerothi]

Jonas has also asked me about this named stuff which seems quite handy. However, I am not happy about its current usage, it is clumsy and not very easy to extend.

I plan on reworking this so it is more sturdy and easier to expand on. I just don't like having preserve all over, since then you would need to tweak sub in case all indices are there etc.
It quickly becomes quite problematic and it ain't easy to figure out what is the right thing to do in some cases.

[pfebrer]

since then you would need to tweak sub in case all indices are there etc.

Well, you could define a set of rules for preserving different attributes. Can you check out the following snippet? (it's pretty long, but it is very general and my point is just the concept, not the actual code).

preserve.py: (the framework for preserving attributes based on rules)

class DontPreserveException(Exception):
    pass

# The default behavior for preserving attributes
def default_preserving_function(old_obj, new_obj, attr=None, force=False, **kwargs):
    
    if force or not hasattr(new_obj, attr):
        try:
            return getattr(old_obj, attr)
        except AttributeError:
            raise DontPreserveException
    else:
        raise DontPreserveException

# The behavior that "names" should have
def preserve_names(old_geom, new_geom, **kwargs):
    import numpy as np
    
    preserved = {}
    
    for name, iAts in old_geom.names.items():
        if (np.array(iAts) < new_geom.na).all():
            preserved[name] = iAts
        
    return preserved

# Define the preserving functions for each class and attribute
preserve_funcs = {
    
    "__default": default_preserving_function,
    
    "Geometry": {
        '__default_preserve': lambda self, obj, attr, **kwargs: "hehe",
        'names': preserve_names,
    }
    
}

# The wrapper factory for building wrappers that preserve attributes
def preserve(*attrs, only_if_present=True, **preservekwargs):
    
    def wrapper(method):
        
        def preserved_method(self, *args, **kwargs):
            
            obj = method(self, *args, **kwargs)
            
            for attr in attrs:
                
                # Don't continue if the attribute is not there in the old object
                if not hasattr(self, attr) and only_if_present:
                    continue
                    
                # Find out which function should we use for preserving
                p_funcs_dict = preserve_funcs.get(obj.__class__.__name__, None)
                
                if p_funcs_dict is None:
                    p_func = preserve_funcs["__default"]
                else:
                    # The default can either be class-specific or the general default
                    default = p_funcs_dict.get("__default_preserve", preserve_funcs["__default"])
                    p_func = p_funcs_dict.get(attr, default)
                
                # Finally, use the function to return the attribute value
                try:
                    val = p_func(self, obj, attr=attr, **preservekwargs)
                    setattr(obj, attr, val)
                except DontPreserveException:
                    # Give the possibility to truncate preservation by raising a DontPreserveException
                    pass
                        
            return obj
        
        return preserved_method
    
    return wrapper

geometry.py: (the geometry class using preserve)

# A dummy geometry class to test it
class Geometry:
    
    def __init__(self, na=70):
        self.na = na
        
    @preserve("names", "very_important_attr")
    def sub(self, na=20):
        # Fake sub method
        return Geometry(na)

other.py: (another class that uses preserve without registering)

#Another class that is not registered and therefore will use the default behavior
class Other:
    
    @preserve("x")
    def do(self):
        return Other()

TEST USER CODE:

geom = Geometry()

geom.names = {
    'Cring': [1,2,4,8],
    'Bottom surface': [0,5,6,10,15],
    'Top surface': [5,18,24,32]
}
geom.very_important_attr = "Please preserve me"

geom = geom.rotate()
print(geom.names)
print(geom.very_important_attr)

You can run this code all together to see how it would work (it is standalone).

I just don't like having preserve all over

I agree that class definition might look a bit crowded, but it would be very explicit.

Another option would be to use this same concept but instead of a wrapper build a function that you could call on return in your method. It would be less explicit (meaning that you would have to look for it in each method to know what is being preserved) but the class definition would look as clean as it is right now.

Sorry for the long snippet, but I just wanted to prove that this could work very generally :)

[zerothi]

Thanks for the long code ;)

I am not worried about generality, I am more worried about maintainability of what you have. One would have to remember this for all methods since each may require a specific sub-set argument. When we are already at this point where one should customize a wrapper for individual methods, then I would still argue it is too messy. This is why I haven't really recommended people to use names since I had a feeling it would require some additional work (just as you have discovered).

[pfebrer]

Hmm, right. Maybe you could track atoms based on ids instead of indices to avoid some problems. But then you'd have to make sure that atoms keep their ids. And you'd need an extra array to store them.

I don't know, but I agree that this feature is very handy so if possible I think you should keep it and make it consistent through operations in some way :)

[jonaslb]

This might be an issue for other methods that return new geometries (not copied ones), I haven't checked.

Perhaps an inplace keyword could be used instead -- then name preservation behaviour could follow it as well?

Eg. so that geom.rotate(...) would be as now, while geom.copy().rotate(..., inplace=True) would preserve names?

[pfebrer]

But it is not a question of doing it inplace is it? Because when a method uses a copied geometry the names are preserved and still you would say it's not inplace. The problem only exists when a new geometry is created.

So, you would say that in the methods you should either do it inplace or generate a new geometry (never use a copied one)?

Anyway, I think an inplace keyword would be great because there may be some cases where you have the geometry referenced in several places and you want the change to affect all these references without you needing to change them explicitly. And also (this is personal), sometimes it is annoying to write geom = geom.method() hahahah.

[jonaslb]

So I at least used to think that methods should mostly preserve names whenever they can. But I had some discussions with Nick over complexities that arise with names when eg. adding geometries. And how to deal with names when tiling etc. Those issues are #116 #117 #118 #119 . I kind of stopped using names since then, as I don't really feel they give much advantage the way they currently are implemented (ie. very scarcely).

At least 'never transfer names to new objects unless using copy' is a very simple strategy and behaviour. And consistency is also nice :) With that argument move ought to be more like rotate rather than the other way around.

[zerothi]

Yes, I plan on reworking the names at some point, I need to add an attribute to classes to enable simple handling of these things. But for now I would be reluctant to adding anything since it will just have to be removed later on... + it adds lots of boiler plate code...