Closed maximelucas closed 8 months ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Attention: 12 lines
in your changes are missing coverage. Please review.
Comparison is base (
2eea477
) 92.00% compared to head (386dfe4
) 91.89%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
🚀 @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)
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
andwidth
params and allowing the user to pass anax
the problem (and reason why I went in that direction originally) is that theax
input parameter needs to be aAxes3D
object and not aAxes
(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?
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
:
sep = 1
:
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
:
sep = 0.4
:
sep = 0.1
:
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.
Ok it's ready I somebody approves then we can merge
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?
Thanks Nich! (1) Yes! (2) Done.
This makes it more consistent with other (see #477).
_scalar_arg_to_dict
solving #442 and #410.I couldn't make
sep
work (related to #433) @thomasrobiglio can you check? Related to that I would maybe remove theheight
andwidth
arguments as users can control that externally and pass anax
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.