search

scikit-hep / hist

Histogramming for analysis powered by boost-histogram
https://hist.readthedocs.io
BSD 3-Clause "New" or "Revised" License
128 stars 25 forks source link

[BUG] operator overloads don't work with the shortcut method. #92

Closed henryiii closed 4 years ago

henryiii commented 4 years ago

If you do repr(Hist.Reg(10,0,1)), that breaks because dunder methods are looked up on the class, not the object.

henryiii commented 4 years ago

This could be manually fixed on a case-by-case basis, but there are enough warts with the current system I'd like to propose a replacement:

Add a new Hist.new class object. That object is the Hist proxy. Calling a storage returns a real Hist instance. Maybe a few (at least fill) methods could be included too that would use the default storage. This would keep the namespaces cleaner, and should be a much simpler implementation.

So:

h: Hist = Hist.new.Reg(3,1,2).Log(10,1,10).Double()
h: Hist = Hist.new.Reg(3,1,2).Log(10,1,10).fill(...) # Fill could automatically create a default storage and start filling it

Tab completion and static typers (mypy, PyCharm, etc) would be helpful in guiding a user to do the right thing; trading a little magic for type safety.

@LovelyBuggies thoughts?

LovelyBuggies commented 4 years ago

I think it makes sense. Do you want to add this change to v2.0.0?

Btw, the CHANGELOG looks humble, we'd better make it more clearly.

henryiii commented 4 years ago

Yes, this is a blocker - having >>> Hist.Reg(10,0,1) broken on the REPL is not something we can allow in 2.0.

The CHANGELOG starts at 2.0 final, so it will not really have any entries till we release 2.0.

henryiii commented 4 years ago

Though I guess you could have it go for the whole life of the project; 2.0 would simply be a list of the current features, and the few 1.0 releases were just the command line tiny program that existed.