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

DEPR: derpecate HaloCatalogCallback, let it migrate to yt_astro_analysis #3567

Merged

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Oct 14, 2021

PR Summary

complementary part of yt-project/yt_astro_analysis#98
addressing yt-project/yt_astro_analysis#93
and fix #3566
This should probably not go in before yt_astro_analysis 1.1 is released, see https://github.com/yt-project/yt_astro_analysis/milestone/2

@neutrinoceros neutrinoceros added the deprecation deprecate features or remove deprecated ones label Oct 14, 2021
@neutrinoceros neutrinoceros added the dead code removing internal bits that have no effect label Oct 14, 2021
@neutrinoceros neutrinoceros marked this pull request as draft October 14, 2021 17:37
@neutrinoceros neutrinoceros force-pushed the remove_halocatalog_callback branch 2 times, most recently from d6a9862 to b000819 Compare October 17, 2021 20:13
@neutrinoceros neutrinoceros marked this pull request as ready for review December 15, 2021 14:26
@matthewturk
Copy link
Member

@brittonsmith I think this needs your stamp of approval

@neutrinoceros
Copy link
Member Author

Checking for this helped me find a bug in yt_astro yt-project/yt_astro_analysis#131

With the associated bugfix + this branch, I've checked that halo plotting still works as documented

@matthewturk matthewturk enabled auto-merge January 11, 2022 16:07
@brittonsmith
Copy link
Member

I've thought about this a bit and I've come back to the opinion that it would be good to have some way to inform the user that this callback has been removed and where to find it now. It's non-trivial enough for the user to figure out how to get this functionality back, I'd rather not remove it with no message. Would it be possible to make a HaloCatalogCallback class with an __init__ function that accepts *args, **kwargs and a __call__ method that just raises an exception with a message about this having moved to yt-astro?

@neutrinoceros
Copy link
Member Author

I experimented with this approach and in summary yes, it is possible.
I've tested 4 different ways to import yt + the astro extensions from the user side and checked that the Halo callback that persists in the registry is always from the extension when available (1.1+) and always from yt otherwise (duh).

So in fact we don't have to make the code here disappear, and we can just add a deprecation warning, which is much better. I'll update.

@neutrinoceros neutrinoceros force-pushed the remove_halocatalog_callback branch from b000819 to fbf6582 Compare January 26, 2022 11:23
@neutrinoceros neutrinoceros changed the title DEPR: remove HaloCatalogCallback, let it migrate to yt_astro_analysis DEPR: derpecate HaloCatalogCallback, let it migrate to yt_astro_analysis Jan 26, 2022
@neutrinoceros neutrinoceros added this to the 4.1.0 milestone Jan 26, 2022
Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Ok, that's great. Thanks!

@matthewturk matthewturk merged commit 18baf57 into yt-project:main Jan 26, 2022
@neutrinoceros neutrinoceros deleted the remove_halocatalog_callback branch January 26, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dead code removing internal bits that have no effect deprecation deprecate features or remove deprecated ones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPR: deprecate annotate_halos (HaloCatalogCallback)
3 participants