-
Notifications
You must be signed in to change notification settings - Fork 276
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
[bugfix] fix io chunking for chained cut regions #2234
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.
minor initialization order change
if isinstance(data_source, YTCutRegion): | ||
self.__init__( | ||
data_source.base_object, | ||
data_source.conditionals + conditionals, |
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 ensure_list(conditionals)
needs to happen before this
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.
Totally right!
if isinstance(data_source, YTCutRegion): | ||
self.__init__( | ||
data_source.base_object, | ||
data_source.conditionals + conditionals, |
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.
Calling __init__
inside of __init__
is a little weird to me. Is it possible to set data_source
and conditionals
in this if block and then set up the instance using the existing logic below?
Also I realize this is how it is already, but a straightforward cleanup would be to deprecate (with a visible warning) passing base_object
in and to call the attribute data_source
instead of base_object
.
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.
Yeah, this can probably be simplified like that. I'll give it a go.
PR Summary
This closes Issue #2233.
When chained cut regions do io chunking, only the conditionals of the last cut region are applied. Conditionals from parent cut regions are ignored. This only shows up when calculating derived quantities.
The solution implemented here changes how cut regions derived from other cut regions are initialized. Instead of creating a chain of objects that rely on each's parent, we add the conditionals from the parent cut region and create the new one from the parent's base object. Anecdotally, this also appears to speed up field access as we're only applying conditionals once.
PR Checklist