Skip to content

Conversation

@scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Dec 21, 2023

PR summary

Addresses #784 in part

More discussion in #19573

Note that the inherited Collection.set_offset and Collection.get_offset methods with 2D coordinates are used in zordering calculations and I couldn't see an easy way to abstract those out in order to override those methods and keep the same names as the 2D case.

PR checklist

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jan 11, 2024

Only test failure is codecov, but these are fairly well covered and should be fine.

@scottshambaugh scottshambaugh added this to the v3.9.0 milestone Mar 1, 2024
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Minor style fixes / improvements.

@timhoffm
Copy link
Member

I took the liberty to create a commit with the style fixes.

@timhoffm
Copy link
Member

timhoffm commented Mar 11, 2024

Could you add a simple test? I think just manipulating the values and reading them back to assert that you get the new offesets is sufficient. Or, if you want to be a bit more fancy make an image comparison test where you create a collection in fig_test with offests_0 and then set_offsets3d(offsets_1) and compare that to fig_ref with directly set offsets_1. This could also be amended with zdir.

@scottshambaugh scottshambaugh marked this pull request as draft March 12, 2024 23:01
@scottshambaugh scottshambaugh removed this from the v3.9.0 milestone Mar 12, 2024
@scottshambaugh
Copy link
Contributor Author

Moving back to draft since I need to dig more into what Poly3DCollection is doing

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.

2 participants