Allow overriding of protocol methods in subclasses

Hi there 👋🏻

I opened this issue to discuss one particular use case we, at MadMiner, are having trouble with.

We have a custom vector class, MadMinerParticle, overriding some of the protocol methods coming from its parent class MomentumVector4D (which, in reality, inherits them from VectorObject). When using one of these methods (our trouble is with __iadd__), we noticed that the operation makes use of the __array_ufunc__ method, which calls the _handler_of dispatch function, obtaining the module in which the provided object class was defined.

vector/src/vector/_methods.py

Lines 2547 to 2553 in d9cd956

    
           if isinstance(obj, Vector): 
        
               if handler is None: 
        
                   handler = obj 
        
               elif _handler_priority.index( 
        
                   type(obj).__module__ 
        
               ) > _handler_priority.index(type(handler).__module__): 
        
                   handler = obj

The problem is that, with a custom VectorObject subclass, the module information points to a Python module that is on the client code, and therefore, not recognized among the valid list of modules.

vector/src/vector/_methods.py

Lines 2529 to 2533 in d9cd956

    
           _handler_priority = [ 
        
               "vector._backends.object_", 
        
               "vector._backends.numpy_", 
        
               "vector._backends.awkward_", 
        
           ]

Could you expand the logic on the _handler_of dispatch function so that, not only the object's type __module__ gets looked up, but the complete list of parent classes in MRO order?

Thanks in advance.

It's not clear to me what you're asking, but if you know how you want it to change, could you try that as a pull request? (It doesn't help that it's been a while since I've looked at this code, but you seem to know exactly what the problem is.)

When I wrote it, I wasn't thinking about what would happen if these classes are subclassed by other libraries. If this turns out to be going against the grain, what about using composition, rather than inheritance? (Just as an idea to consider.)

We could use composition, however, that would imply that the similarity between how client code used to look when using scikit-hep < 1.0.0 embedded vector functionality, and how it looks now when using the vector package alone decreases.

Before, the protocol methods did not infer classes modules, so we had no problems calling LorentzVector __iadd__ and __isub__ from our MadMinerParticle custom class. Now, we are running into trouble.

I will propose a PR to show what I had in mind. It should be a simple fix.

Feel free to consider it, or discard it if you think it is unnecessary.

@Sinclert

We could use composition, however, that would imply that the similarity between how client code used to look when using scikit-hep < 1.0.0 embedded vector functionality, and how it looks now when using the vector package alone decreases.

Before, the protocol methods did not infer classes modules, so we had no problems calling LorentzVector __iadd__ and __isub__ from our MadMinerParticle custom class. Now, we are running into trouble.

I will propose a PR to show what I had in mind. It should be a simple fix.

Feel free to consider it, or discard it if you think it is unnecessary.

Hi @Sinclert, you mentioning scikit-hep reminds me of an old chat related to your package depending on it - is that still the case? If not then one could very soon deprecate and remove the vector module and relatives from scikit-hep. Advance thanks.

Could we add the deprecated package to scikit-hep (the package) and print warnings when people use things like vectors?

Could we add the deprecated package to scikit-hep (the package) and print warnings when people use things like vectors?

Seems a good idea to me 👍. Today for sure not but I will try and have a look into what can be deprecated asap ...

@Sinclert

We could use composition, however, that would imply that the similarity between how client code used to look when using scikit-hep < 1.0.0 embedded vector functionality, and how it looks now when using the vector package alone decreases.
Before, the protocol methods did not infer classes modules, so we had no problems calling LorentzVector __iadd__ and __isub__ from our MadMinerParticle custom class. Now, we are running into trouble.
I will propose a PR to show what I had in mind. It should be a simple fix.
Feel free to consider it, or discard it if you think it is unnecessary.

Hi @Sinclert, you mentioning scikit-hep reminds me of an old chat related to your package depending on it - is that still the case? If not then one could very soon deprecate and remove the vector module and relatives from scikit-hep. Advance thanks.

Hi @eduardo-rodrigues ,

MadMiner was depending on scikit-hep until very recently, but it only used the vector functionality from it. I ended up simplifying the dependency to just vector (reduced scope), so feel free to add any warnings you want to the scikit-hep vector module.

@Sinclert

Morning @Sinclert, sounds good, and makes sense, of course.

Your comment above made me look out of curiosity at https://github.com/diana-hep/madminer/blob/2e39992dfdf15f73574a8885bd9c3bfa3a63417a/madminer/utils/particle.py#L25-L40. It's a bit of a shame that you hardcode everything instead of relying on the Particle package. Most importantly, those lines seem to have many charge assignments wrong! Just check with Particle.from_pdgid(self.pdgid).charge ... Indeed, assuming that self.pdgid = 1 is also the down quark in your package (why would it not be!?), then the charge is -1/3 and not 2/3 as your code indicates. Similar issues for all quark charges.
In short, better almost always to not hardcode and rely on validated code/packages ;-).

BTW, you can for example look at all quarks with Particle.findall(lambda p: p.pdgid.is_quark).

@eduardo-rodrigues

Hi @eduardo-rodrigues ,

I completely agree with you, using validated code and packages should be the norm, and I will be happy to try the particle package. Please, bear in mind that most of the code base (including the snippet you referenced) was written by Johann Brehmer and Felix Kling, I joined the team late in the development, and from a technology background.

In short, I do not know what those ranges mean and why they are used to infer particle charge.

No worries, all understood :). But better cross-check that piece of code with your colleagues as it can all be reduced to the one liner above and the present code is very wrong if I read correctly.

Navigation Menu

Repository navigation

Allow overriding of protocol methods in subclasses #127

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if isinstance(obj, Vector):
	if handler is None:
	handler = obj
	elif _handler_priority.index(
	type(obj).__module__
	) > _handler_priority.index(type(handler).__module__):
	handler = obj

	_handler_priority = [
	"vector._backends.object_",
	"vector._backends.numpy_",
	"vector._backends.awkward_",
	]

Allow overriding of protocol methods in subclasses #127

Description

Activity

jpivarski commented on Jul 29, 2021

Sinclert commented on Jul 29, 2021

eduardo-rodrigues commented on Aug 17, 2021

henryiii commented on Aug 17, 2021

eduardo-rodrigues commented on Aug 17, 2021

Sinclert commented on Aug 18, 2021

eduardo-rodrigues commented on Aug 18, 2021

eduardo-rodrigues commented on Aug 18, 2021

Sinclert commented on Aug 18, 2021

eduardo-rodrigues commented on Aug 18, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions