Skip to content

Conversation

samwaseda
Copy link
Member

I added the box frame in plot3d for plotly.

@samwaseda samwaseda requested review from liamhuber and pmrv August 8, 2023 09:45
@samwaseda samwaseda changed the title Show box cell in poorly Show box cell in plotly Aug 8, 2023
Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

I'm doubling-down on Marvin's nits, but they are not mission-critical IMO. Take the feedback seriously but merge when you're content.

@samwaseda
Copy link
Member Author

Since there's nothing that breaks will most likely also never break anything, I'm gonna merge it now. Feel free to open another PR for more changes.

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

Needs some clarity for those of us not fluent in numpy sorcery, but no fundamental objections from me.

Comment on lines 26 to 35
self.assertLessEqual(
np.unique(frame.reshape(-1, 6), axis=0, return_counts=True)[1].max(),
1
)
dx, counts = np.unique(
np.diff(frame, axis=-2).squeeze().astype(int), axis=0, return_counts=True
)
self.assertEqual(dx.ptp(), 1)
self.assertEqual(counts.min(), 4)
self.assertEqual(counts.max(), 4)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what any of these are checking, nor why they should return the integers they do. They don't necessarily need any change, but please specify msg= to clarify.

@liamhuber
Copy link
Member

Since there's nothing that breaks will most likely also never break anything, I'm gonna merge it now. Feel free to open another PR for more changes.

Ah, I saw it in my inbox and jumped straight to the review without seeing this or even realizing I'd looked at the PR before. I think we're on the same page though, as my review is basically only asking for comments and no code changes, so I agree the code itself is mergeable. (Well, except for the method rename, but we don't need that part)

samwaseda and others added 2 commits February 8, 2024 14:21
Co-authored-by: Liam Huber <liamhuber@greyhavensolutions.com>
@samwaseda samwaseda merged commit 636a691 into main Feb 8, 2024
@samwaseda samwaseda deleted the plotly_draw_frame branch February 8, 2024 21:31
@samwaseda
Copy link
Member Author

Comment: Apparently this feature moves the legend slightly. It's probably because plotly creates space for the extra lines in the box. So far it doesn't look like it makes a problem, but someone might want to take a look at it at later point if there's a case arising due to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants