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

DM-41700: Deprecate Naive Centroid #268

Merged
merged 2 commits into from Apr 15, 2024
Merged

DM-41700: Deprecate Naive Centroid #268

merged 2 commits into from Apr 15, 2024

Conversation

parejkoj
Copy link
Contributor

@parejkoj parejkoj commented Apr 1, 2024

No description provided.

class NaiveCentroidControl {
class [[deprecated(
"This algorithm and its control class are deprecated and will be removed after "
"v27.")]] NaiveCentroidControl {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be best to suppress these warnings in the pybind11 module so we don't see them when compiling that also-deprecated code. The preprocessor directives to do that can be found in the dev guide just above your new paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I had to put the pragma around the entire pybind11 block in the namespace, but that did take care of the compiler warnings.

@@ -23,6 +23,6 @@
from _sillyCentroid import SillyCentroidAlgorithm, SillyCentroidControl, SillyTransform

# Do not register SillyCentroid in plugins.py, as it's not part of meas_base
wrapSimpleAlgorithm(SillyCentroidAlgorithm, Control=SillyCentroidControl,
wrapSimpleAlgorithm(SillyCentroidAlgorithm, Control=SillyCentroidControl, name="testLib_SillyCentroid",
Copy link
Member

Choose a reason for hiding this comment

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

I think this algorithm is just on the border of being simple enough that it might have saved time to reimplement it in pure Python. But now that you've done it this way that's probably not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it took me a bit to sort out the various "testLib" references (and lack thereof), but this works fine. Having someone implement SillyCentroid (which is basically identical to NaiveCentroid) in python might be a useful training exercise for a newer developer, but is otherwise not worth it.

* Add new deprecation system for registered C++ plugins.
* Use pure-test-based SillyCentroid to replace NaiveCentroid in tests.
* Mark code to remove on DM-41701, once the deprecation period has passed.
@parejkoj parejkoj merged commit 212a7c6 into main Apr 15, 2024
2 checks passed
@parejkoj parejkoj deleted the tickets/DM-41700 branch April 15, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants