Add VarVsVar object

[dkobylianskii]

Summary

The idea is to create a VarVsVar object for plotting a variable vs another variable with the std error, as suggested in #136.
Later we can use it as a base class for VarVsEff.

It might be helpful in some cases, such as MC-MC plotting.

Example output:

@samvanstroud what do you think?

Conformity

• Changelog entry
• Documentation

[codecov-commenter]

Codecov Report

Merging #172 (8366fc2) into main (b764bbc) will increase coverage by 0.24%.
The diff coverage is 99.53%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   96.25%   96.50%   +0.24%     
==========================================
  Files          30       32       +2     
  Lines        2646     2859     +213     
==========================================
+ Hits         2547     2759     +212     
- Misses         99      100       +1     

Impacted Files	Coverage Δ
puma/var_vs_var.py	99.22% <99.22%> (ø)
puma/__init__.py	100.00% <100.00%> (ø)
puma/line_plot_2d.py	100.00% <100.00%> (ø)
puma/tests/test_var_vs_var.py	100.00% <100.00%> (ø)
puma/utils/__init__.py	72.50% <100.00%> (+1.44%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[dkobylianskii]

I added get_good_markers function to automatically select the marker for VarVsVar plot and LinePlot2D
Fill and markers now are customizable

[samvanstroud]

This is awesome @dkobylianskii !

The implementation looks good and I agree with the plan to use this as the base class for the efficiency profile plots in the future. Shall we call the class ProfilePlot, and then later rename also VarVsEff to EfficiencyProfile or similar? Probably better to use the standard terminology for these plots.

I also wonder if we can add a convenience function to compute the binned values before plotting from the raw points using e.g. a DataFrame groupby or binned_statistic_2d. This could be left for a future MR though as I think this is already a nice addition!

[afroch]

Hi @dkobylianskii. Looks already really good. I just have one suggestion (which can be easily added) and one comment. The suggestion is already marked and the comment is, that you need to add unit tests for this new type of plot. This would also fix the coverage again.

[afroch]

Hi @dkobylianskii. Looks good! If you could just add a changelog entry, I think we are good to go!

[dkobylianskii]

Hi @dkobylianskii. Looks good! If you could just add a changelog entry, I think we are good to go!

Added

[samvanstroud]

Hi @dkobylianskii, thanks for the updates. Do you prefer to leave these items to follow up MRs? If so would you be able to open an issue to keep track? Thanks :)

The implementation looks good and I agree with the plan to use this as the base class for the efficiency profile plots in the future. Shall we call the class ProfilePlot, and then later rename also VarVsEff to EfficiencyProfile or similar? Probably better to use the standard terminology for these plots.

I also wonder if we can add a convenience function to compute the binned values before plotting from the raw points using e.g. a DataFrame groupby or binned_statistic_2d. This could be left for a future MR though as I think this is already a nice addition!

[dkobylianskii]

Hi @dkobylianskii, thanks for the updates. Do you prefer to leave these items to follow up MRs? If so would you be able to open an issue to keep track? Thanks :)

The implementation looks good and I agree with the plan to use this as the base class for the efficiency profile plots in the future. Shall we call the class ProfilePlot, and then later rename also VarVsEff to EfficiencyProfile or similar? Probably better to use the standard terminology for these plots.

I also wonder if we can add a convenience function to compute the binned values before plotting from the raw points using e.g. a DataFrame groupby or binned_statistic_2d. This could be left for a future MR though as I think this is already a nice addition!

Hi @samvanstroud ,
I think that we can leave it for future MR. I started to look how to use it as a base class, but we need to discuss the details.
I will open the issue regrading this.

[samvanstroud]

@afroch you might also need to approve the requested changes