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

Remove hardcoded alpha from hyperedge drawing #347

Closed
wants to merge 1 commit into from
Closed

Conversation

leotrs
Copy link
Collaborator

@leotrs leotrs commented May 3, 2023

The transparency of hyperedges is currently hardcoded to 0.4.

The following code:

import xgi
import matplotlib.pyplot as plt
H = xgi.Hypergraph([[1,2,3], [2,3,4]])
xgi.draw(H, edge_fc=[[0,0,1,1], [0,0,1,0.1]])
plt.show()

Before this PR:

After this PR:

Cc. @iaciac

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.03 🎉

Comparison is base (5497778) 90.36% compared to head (16793a1) 90.39%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
+ Coverage   90.36%   90.39%   +0.03%     
==========================================
  Files          41       41              
  Lines        2988     2988              
==========================================
+ Hits         2700     2701       +1     
+ Misses        288      287       -1     
Impacted Files Coverage Δ
xgi/drawing/draw.py 72.39% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nwlandry
Copy link
Collaborator

nwlandry commented May 3, 2023

I like the concept of this, but I picture alpha being a separate parameter with a default value of 0.4, instead of lumped with the color. As it is implemented in this PR, drawing with the default parameters, i.e., xgi.draw(H), the edges will not be transparent at all, right?

@leotrs
Copy link
Collaborator Author

leotrs commented May 3, 2023

Yeah you're right.

I will say however that specifying the alpha of each color is actually very common.

Right now, the default value of 0.4 overrides any alpha value in the color. The current PR fixes this. I can also try to make it so that we accept alpha values as a separate array, separate from color. I honestly do not like that idea at all since then there would be two different ways of specifying the same thing.

@nwlandry
Copy link
Collaborator

nwlandry commented May 3, 2023

I see. So is there a way to have a default alpha entry for facecolor which gets overridden when the user specifies it?

@maximelucas
Copy link
Collaborator

I'm unsure what I'd want here too.

I understand you can specify an alpha for each color as part of the tuple (like in Leo's example above).
At the same time, many functions in matplotlib, like plt.plot, take alpha as an argument for all objects plotted.

I feel like people would less often want to specify different alpha for different edges, rather than a single alpha for all of them.

I agree with Nich that it would be nice to have a default alpha that can be overriden.

@leotrs
Copy link
Collaborator Author

leotrs commented May 4, 2023

Yeah you're right. In light of this being what MPL does, we should follow suit too.

@leotrs
Copy link
Collaborator Author

leotrs commented May 4, 2023

Thanks both for your comments. I think having a default alpha value is nice too.

After playing with plt.scatter, it seems like:

  1. specifying alpha as part of the color is supported,
  2. specifying alpha as a standalone optional argument alpha=0.5 is supported,
  3. if both are specified, the latter overrides the former (i.e. all elements are plotted with the same alpha value, even if they each have different alpha values as part their color).
  4. the argument alpha=0.5 can only receive one float, i.e. you cannot pass in an array specifying different alpha values, one for each element.

This is counterintuitive to me for many reasons, but whatever. Do we want to mirror the same interface?

@nwlandry
Copy link
Collaborator

nwlandry commented May 5, 2023

Thanks both for your comments. I think having a default alpha value is nice too.

After playing with plt.scatter, it seems like:

  1. specifying alpha as part of the color is supported,
  2. specifying alpha as a standalone optional argument alpha=0.5 is supported,
  3. if both are specified, the latter overrides the former (i.e. all elements are plotted with the same alpha value, even if they each have different alpha values as part their color).
  4. the argument alpha=0.5 can only receive one float, i.e. you cannot pass in an array specifying different alpha values, one for each element.

This is counterintuitive to me for many reasons, but whatever. Do we want to mirror the same interface?

I think this plan makes sense for now. If we handle the logic flow that you've described in _color_arg_to_dict() we can (potentially?) always bundle the alpha value with the color when we convert to a dict.

@maximelucas
Copy link
Collaborator

Do we want to mirror the same interface?

I'd say let's mirror this for now yes

@maximelucas
Copy link
Collaborator

I think this was solved with #456. If you're happy with this @leotrs we can close.

@nwlandry
Copy link
Collaborator

nwlandry commented Nov 1, 2023

I'm closing this since this was solved with #456. @leotrs can re-open this if necessary.

@nwlandry nwlandry closed this Nov 1, 2023
@nwlandry nwlandry deleted the fix-alpha branch December 4, 2023 19:03
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.

3 participants