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

RFC: rewrite find_lowest_subclasses to better match its intent #4411

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

yut23
Copy link
Member

@yut23 yut23 commented Apr 10, 2023

PR Summary

This implementation is more straightforward than the original one with fewer edge cases that need special handling.
It ended up being quite similar to the final implementation in #4405, except it can handle having the same class show up in candidates more than once.
It simply makes a set of all the superclasses of the candidates, and filters out any candidates that appear in that set.

@neutrinoceros
Copy link
Member

It ended up being quite similar to the final implementation in #4405, except it can handle having the same class show up in candidates more than once.

I'm not against dropping my PR in favour of this one in principle, but could explain why this extra bit of behaviour is needed or desired ?

@yut23
Copy link
Member Author

yut23 commented Apr 10, 2023

It's not needed for any existing code, but I think it's a bit more future-proof. I'm fine with either implementation (the way they work is identical except for that edge case), but the extra tests should be added either way.

@neutrinoceros
Copy link
Member

Actually I cannot find an implementation that doesn't survive your new tests (both #4405 and the main branch seem to pass them). Since your original post suggests you're adding new behaviour, this may indicate that you're not actually testing what you aimed for.
These are super quick tests so I don't mind adding them to the code base even if they turn out to be redundant, but maybe you'd like to revisit them first ?

@yut23
Copy link
Member Author

yut23 commented Apr 10, 2023

test_empty was an error I ran into while writing this: the full unit tests caught it, but I figured it's good to explicitly test it here; test_without_grandparents is just another possible edge case that was resolved by #4397.

I could add a test for a class being in candidates multiple times, but I didn't even consider it as a feature when I was writing the code. I only noticed that when I compared it to your implementation.

@neutrinoceros
Copy link
Member

I think it's safe that this function assumes unicity of candidates, though maybe it should be mentioned in the docstring.

@neutrinoceros neutrinoceros added the refactor improve readability, maintainability, modularity label Apr 11, 2023
@neutrinoceros
Copy link
Member

@yut23 I think we could merge your tests and my implementation. Would that work for you ?

@yut23
Copy link
Member Author

yut23 commented Apr 12, 2023

Sounds good to me!

@neutrinoceros
Copy link
Member

Then could you include my implementation here ? This seems like the easiest way to avoid conflicts.
What else is keeping this from leaving draft mode ?

@yut23 yut23 force-pushed the rework_find_lowest_subclasses branch from b083bdf to 15487c0 Compare April 12, 2023 14:22
@yut23 yut23 marked this pull request as ready for review April 12, 2023 14:23
@neutrinoceros neutrinoceros merged commit cba3f8c into yt-project:main Apr 14, 2023
@yut23 yut23 deleted the rework_find_lowest_subclasses branch May 18, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor improve readability, maintainability, modularity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants