-
Notifications
You must be signed in to change notification settings - Fork 272
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
BUG: add support for setting up derived fields from lambdas #3440
BUG: add support for setting up derived fields from lambdas #3440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good.
@@ -250,10 +250,7 @@ def get_dependencies(self, *args, **kwargs): | |||
This returns a list of names of fields that this field depends on. | |||
""" | |||
e = FieldDetector(*args, **kwargs) | |||
if self._function.__name__ == "<lambda>": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've done some thinking, and I think this comes from waaaay back in the day when we may have used lambdas to represent on-disk fields.
Thanks Matt, now I'm much more confident in calling this a plain bug fix :) |
@jzuhone , @matthewturk , anyone in particular that you would like to take a look a this too or is it good for merging ? |
@meeseeksdev backport to yt-4.0.x |
…fields from lambdas
…0-on-yt-4.0.x Backport PR #3440 on branch yt-4.0.x (FIX: add support for setting up derived fields from lambdas)
PR Summary
This is an alternative fix to #3434, and supersedes #3438.
The solution tried here was suggested by @forrestglines. It's not clear to me why the problematic check was ever necessary but it's
been here for 8yrs so my hypothesis is that it may have become inappropriate in Python 3 ?
I'm doubting this would break anything since there doesn't seem to be a single use case of lambdas in derived fields in the code base/docs/tests. On the other hand, the test I'm adding here fails on the main branch (as reported in #3434).