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.

Sign up for GitHub

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

Jump to bottom

Move _invert_no_zero to caput and add deprecation warning #202

Merged
merged 1 commit into from
Nov 8, 2022
Merged

Move _invert_no_zero to caput and add deprecation warning #202

merged 1 commit into from
Nov 8, 2022

Conversation

ljgray
Copy link
Contributor

@ljgray ljgray commented Sep 29, 2022

@ljgray ljgray mentioned this pull request Sep 29, 2022
Merged
@ljgray ljgray force-pushed the invert-no-zero branch 2 times, most recently from 7de9535 to d9c64b8 Compare November 1, 2022 18:20
@jrs65
Copy link
Contributor

jrs65 commented Nov 1, 2022

I think this is linked to the other PR, but we should move the routine at the level of the draco.util.tools.invert_no_zero rather than the _fast_tools._invert_no_zero level.

@ljgray
Copy link
Contributor Author

ljgray commented Nov 1, 2022

I think this is linked to the other PR, but we should move the routine at the level of the draco.util.tools.invert_no_zero rather than the _fast_tools._invert_no_zero level.

What I'm thinking here is that we would just remove invert_no_zero from draco altogether, and this is just temporary so that anyone using it can switch to importing from caput. If you think it's fine to make a breaking change then we could just remove it altogether

@jrs65
Copy link
Contributor

jrs65 commented Nov 1, 2022

Agreed. But you should remove _invert_no_zero entirely, move the remaining functionality of draco.util.tools.invert_no_zero into caput, and turn draco.util.tools.invert_no_zero into a thin shim calling caput.tools.invert_no_zero.

@ljgray
Copy link
Contributor Author

ljgray commented Nov 1, 2022

Ah, I see what you mean

@ljgray ljgray force-pushed the invert-no-zero branch 3 times, most recently from 1aa360c to 8ab29fa Compare November 1, 2022 19:24
@ljgray ljgray marked this pull request as ready for review November 2, 2022 00:02
@ljgray ljgray merged commit 2cafe5b into master Nov 8, 2022
@ljgray ljgray deleted the invert-no-zero branch November 8, 2022 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrs65 jrs65 jrs65 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ljgray @jrs65