Adding the basic functionality of FilledHistogram class

[Choklatier]

Summary

This pull request introduces the following changes

• Added a FilledHistogram class that takes in bin_edges and bin_heights instead of "values" and weights.
• The FilledHistogram objects can be added to HistogramPlot objects the same way Histogram objects get added.
• New helper function has been added to be used for FilledHistogram uncertainty computation.
• HistogramPlot plot method has been modified to act differently based on the Histogram type.

Relates to the following issues
*Probably the structure should be revisited to get rid of some helper functions and copy of the "divide" method
*Need to implement error handling (throwing and catching stuff) !

Conformity

• Changelog entry
• Documentation

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (12fd171) 97.28% compared to head (5535428) 97.28%.
Report is 1 commits behind head on main.

❗ Current head 5535428 differs from pull request most recent head 058c94c. Consider uploading reports for the commit 058c94c to get more accurate results

Files	Patch %	Lines
puma/histogram.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #221   +/-   ##
=======================================
  Coverage   97.28%   97.28%           
=======================================
  Files          35       35           
  Lines        3383     3420   +37     
=======================================
+ Hits         3291     3327   +36     
- Misses         92       93    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zcapjdb]

Thanks @Choklatier! I think with adding this there are two approaches, extending the current Histogram class to support bin heights and edges or by defining an extra class like you have done here. I'd maybe be more partial to having it all within the original class but that's just personal preference. But I notice with the current setup that there is quite a bit of shared code between the two classes in the __init__ , so maybe a better approach is to define a histogram base class and have the two types of histograms inherit from them to reduce the amount of shared code?

[Choklatier]

Thanks @Choklatier! I think with adding this there are two approaches, extending the current Histogram class to support bin heights and edges or by defining an extra class like you have done here. I'd maybe be more partial to having it all within the original class but that's just personal preference. But I notice with the current setup that there is quite a bit of shared code between the two classes in the __init__ , so maybe a better approach is to define a histogram base class and have the two types of histograms inherit from them to reduce the amount of shared code?

Thank you for the comment! I agree that there is a lot of shared code in the __init__ and also the divide method is copy pasted since it should exist for all histograms. Do you think it is better to have only one Histogram class and handle the differences in the constructor?

[zcapjdb]

Thanks @Choklatier! I think with adding this there are two approaches, extending the current Histogram class to support bin heights and edges or by defining an extra class like you have done here. I'd maybe be more partial to having it all within the original class but that's just personal preference. But I notice with the current setup that there is quite a bit of shared code between the two classes in the __init__ , so maybe a better approach is to define a histogram base class and have the two types of histograms inherit from them to reduce the amount of shared code?

Thank you for the comment! I agree that there is a lot of shared code in the __init__ and also the divide method is copy pasted since it should exist for all histograms. Do you think it is better to have only one Histogram class and handle the differences in the constructor?

Yeah, if that's good with you I think it would be nice that way, just determine which is which based on the constructor arguments

[afroch]

Hi @Choklatier. The PR looks already really good! I just have two comments, mainly about some service stuff.

• Could you please fix the linting and pre-commit issues in the PR?
• Could you also please add tests for your new functionality?
• Could you add a changelog entry?

Once this is done, I'm happy to make the final review and merge it

[zcapjdb]

Nice @Choklatier, just a couple more comments on potentially reducing code duplication but it looks good to me! Feel free to post an example showing it working in action too

[Choklatier]

Hi @Choklatier. The PR looks already really good! I just have two comments, mainly about some service stuff.

* Could you please fix the linting and pre-commit issues in the PR?

* Could you also please add tests for your new functionality?

* Could you add a changelog entry?

Once this is done, I'm happy to make the final review and merge it

I will add the new modification suggested by @zcapjdb and then I'll try to write some tests for the new functionality! I'll let you know if I figure out how to fix the linting and pre-commit issues!

[Choklatier]

Hi @Choklatier. The PR looks already really good! I just have two comments, mainly about some service stuff.

* Could you please fix the linting and pre-commit issues in the PR?

* Could you also please add tests for your new functionality?

* Could you add a changelog entry?

Once this is done, I'm happy to make the final review and merge it

@afroch I believe that each point is done now. Let me know if I missed something or if there is an issue!

[samvanstroud]

This looks great, thanks a lot!

[samvanstroud]

I'm merging this now, thanks again @Choklatier for the great contribution and @zcapjdb and @afroch for the reviews