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

Rewrote the multilayer function #486

Merged
merged 17 commits into from
Oct 29, 2023
Merged

Rewrote the multilayer function #486

merged 17 commits into from
Oct 29, 2023

Conversation

maximelucas
Copy link
Collaborator

@maximelucas maximelucas commented Oct 27, 2023

This makes it more consistent with other (see #477).

I couldn't make sep work (related to #433) @thomasrobiglio can you check?
Related to that I would maybe remove the height and width arguments as users can control that externally and pass an ax of their choice to the function.

The function in general could some more tests but I would leave that for a future PR. I created a new notebook that showcases many use cases and they work.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

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

Comparison is base (2eea477) 92.00% compared to head (386dfe4) 91.89%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #486      +/-   ##
==========================================
- Coverage   92.00%   91.89%   -0.12%     
==========================================
  Files          60       60              
  Lines        4428     4441      +13     
==========================================
+ Hits         4074     4081       +7     
- Misses        354      360       +6     
Files Coverage Δ
xgi/drawing/draw_utils.py 90.90% <ø> (+2.57%) ⬆️
xgi/drawing/draw.py 76.90% <84.21%> (-1.04%) ⬇️

... and 1 file with indirect coverage changes

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

@thomasrobiglio
Copy link
Collaborator

🚀 @maximelucas incredible!

About the sep parameter you mean have a look if it works in the original function?

About removing the height and width params and allowing the user to pass an ax the problem (and reason why I went in that direction originally) is that the ax input parameter needs to be a Axes3D object and not a Axes (as in the other functions for potting)

@maximelucas
Copy link
Collaborator Author

About the sep parameter you mean have a look if it works in the original function?

So in the original function I remember I thought it wasn't working but then you managed to see it work by using small values I think. Here I'm not managing to see any change when I change sep. Can you maybe check if you manage to see changes like you did before, or otherwise help find why mine is different from yours for that? I'm using sep everywhere to set the height.. I looked but I haven't understood why it wouldn't move yet. I even played with height.

About removing the height and width params and allowing the user to pass an ax the problem (and reason why I went in that direction originally) is that the ax input parameter needs to be a Axes3D object and not a Axes (as in the other functions for potting)

Sorry I wasn't with what I meant. Right now we have width and height as arguments and

if ax is None:
        _, ax = plt.subplots(figsize=(width, height), subplot_kw={"projection": "3d"})

what I mean is we could replace it by this which works as well:

if ax is None:
        _, ax = plt.subplots(subplot_kw={"projection": "3d"})

and then we can also remove the width and height arguments. Then indeed, if people want to use a custom axis an control width and height they need to pass a 3d one, but that's okay it's in the doc, they should know. In other words, we don't have width and height arguments for any other draw functions, and other that needing to know it should be 3d, this function doesn't to be different I think?

@maximelucas
Copy link
Collaborator Author

Okay I know what's happening with the sep. It works, but we are rescaling the z-axis so we don't see any difference (I temporarily displayed the spines):

sep = 0.1:
Screenshot 2023-10-27 at 19 35 59

sep = 1:
Screenshot 2023-10-27 at 19 37 13

It appears that changing the height of the axis does not change much: matplotlib ensures the 3d plot looks like a cube.

We can fix this by doing ax.set_aspect("equal"):

sep = 1:

Screenshot 2023-10-27 at 19 42 50

sep = 0.4:
Screenshot 2023-10-27 at 19 43 57

sep = 0.1:
Screenshot 2023-10-27 at 19 45 10

@thomasrobiglio
Copy link
Collaborator

Thanks for the clarifications @maximelucas , good catch about sep and also removing the two parameters for the figure size looks like a much better solution than what i'd originally did.

@maximelucas
Copy link
Collaborator Author

Ok it's ready I somebody approves then we can merge

@nwlandry
Copy link
Collaborator

I had a quick look, and great work! I will defer final approval to Thomas since he has more expertise here, but two quick questions: (1) draw_hypergraph_hull is the last function that references the _color_arg_to_dict function, right? (2) does it make sense to remove the tutorials folder here or in a separate PR?

@maximelucas
Copy link
Collaborator Author

Thanks Nich! (1) Yes! (2) Done.

Copy link
Collaborator

@thomasrobiglio thomasrobiglio left a comment

Choose a reason for hiding this comment

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

Looks good @maximelucas ! thank you very much 🚀

@maximelucas maximelucas merged commit de8a39c into main Oct 29, 2023
18 of 19 checks passed
@maximelucas maximelucas deleted the multilayer branch October 29, 2023 22:47
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