Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate optional plotting capabilities into ashist() #636

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

willcollins10
Copy link
Contributor

Description

This PR integrates optional plotting capabilities directly into the ashist() function. Users can now generate plots ('bar', 'line', or 'step') by setting the plot parameter to True and customize plot appearance using plot_kwargs.

Changes Made

  • Added plot and plot_kwargs parameters to the ashist() function.
  • Implemented plotting logic using matplotlib.pyplot based on the specified plot type.
  • Automatically sets axis labels and plot titles using metadata attributes.
  • Adds vertical lines for bin edges if bin_edges is True.

Testing

  • Added unit tests to validate different plot types and plot_kwargs.
  • Ensured that invalid plot types raise appropriate errors.
  • Verified interaction with existing parameters like density and bin_edges.

Related Issues

Closes #<606>

@willcollins10 willcollins10 marked this pull request as ready for review December 8, 2024 20:18
@nwlandry
Copy link
Collaborator

nwlandry commented Dec 9, 2024

@willcollins10 --- thanks for the PR! I will try to take a look this afternoon!

@nwlandry
Copy link
Collaborator

nwlandry commented Dec 9, 2024

@willcollins10 --- side note, the tests fail because the example you added in the docstring never defines H.

Copy link
Collaborator

@nwlandry nwlandry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks awesome! Three main comments: (1) move the plotting contents to a helper function and (2) add the ability to plot a scatterplot, and (3) fix the docstring so that the doctests pass. Thanks for the contribution!

# Only execute plotting code if plot is True or a string
if plot:
try:
import matplotlib.pyplot as plt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If users have installed xgi, I believe that they will also have matplotlib by default, so not sure that you need to check this.

df = hist(self.asnumpy(), bins, bin_edges, density, log_binning)

# Only execute plotting code if plot is True or a string
if plot:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you break this out into a modular helper function with an underscore prefix like _plot_hist or similar?

Originally from https://github.com/jkbren/networks-and-dataviz
Examples
--------
>>> H.nodes.degree.ashist(plot='bar', plot_kwargs={'color': 'red'})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define what H is. maybe a small example? like

H = xgi.Hypergraph([[1, 2, 3], [3, 4], [1, 3, 6]])
H.nodes.degree.ashist(plot='bar', plot_kwargs={'color': 'red'})

fig, ax = plt.subplots()

# Plot based on type
if plot_type == "bar":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a "scatter" option or replace the "line" option with a scatterplot only plotting the points?

xgi/stats/__init__.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants