ParmEd / ParmEd

Parameter/topology editor and molecular simulator
https://parmed.github.io/ParmEd
382 stars 148 forks source link

Problem with `__getitem__()` when using inheritance and required arguments #1260

Open bradakstx opened 2 years ago

bradakstx commented 2 years ago

I tried inheriting from AmberParm while adding some arguments and adding required positional arguments to the constructor. This works just fine with using super().__init__, etc. However, when generating a new object using brackets, this leads to a signature error inside Structure.__getitem__(). Here's a MWE using the example files:

from parmed.amber import AmberParm

class NewParm(AmberParm):
    # Make prm_name and xyz required, preclude usage of box.
    def __init__(self, prm_name, xyz, new_req_arg, *args, **kwargs):
        super().__init__(prm_name, xyz, box=None, *args, **kwargs)
        self.new_attr = new_req_arg

prm_name = 'ParmEd/examples/amber/ala2_solv.parm7'
xyz = 'ParmEd/examples/amber/ala2_solv.rst7'

foo = NewParm(prm_name, xyz, 'foo')
bar = foo[':1']

The relevant traceback parts are:

    bar = foo[':1']
  /parmed/amber/_amberparm.py", line 375, in __getitem__
    other = super(AmberParm, self).__getitem__(selection)
  /parmed/structure.py", line 1082, in __getitem__
    struct = type(self)()
TypeError: __init__() missing 3 required positional arguments: 'prm_name', 'xyz', and 'new_req_arg'

I think this is a fixable issue(?) or else maybe it's a feature and the correct way to do this is with a default arguments or maybe a classmethod? I guess the problem is that __getitem__ requires the possibility of a blank constructor? At the very least we can probably improve the error message as to what is truly wrong.

swails commented 2 years ago

Yes, the way __getitem__ is implemented requires subclasses to accept an empty constructor. You can override __getitem__ to do the right thing and pass the right arguments as well.

Alternatively you could implement an instance method on Structure like _instantiate_empty_structure that takes no arguments and has a default implementation returning type(self)() that you override in your subclass. That would be a minimally invasive fix that would allow you to override that limitation in your subclass.