-
Notifications
You must be signed in to change notification settings - Fork 12
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
Introduce decorator for broadcasting 1D methods #397
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
a56f748
to
ccaa458
Compare
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.
Thanks a lot for this monumental piece of decorator wizardry @willGraham01!
I like the idea, and find it well tested and meticulously documented.
Most of my comments are minor and purely cosmetic in nature.
My more substantial feedback has to do with making the example conceptually simpler, and somewhat shorter. Broadcasting and decorators are tough concepts to wrap one's head around, even for me. If we want users to benefit from the power of these decorators, we need to make them easy to understand (or as easy as we can). The success of this feature will mostly hinge on the quality of the example, which is why I've focused my feedback there.
As you'll see in the relevant comment, I have two proposals for reducing the length and the conceptual complexity of the example:
- use one of our existing sample datasets instead of constructing your own at the start
- motivate the problem with something simpler than camera FOV. Though this application is very cool and relevant for the audience, having to think about angles and trigonometry detracts from the main message of this example, at least in my opinion.
) | ||
|
||
xr.testing.assert_equal(was_in_view_da_broadcasting, was_in_view_space_only) | ||
|
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.
Maybe also mentione the space_broadcastable
alias here.
fig.show() | ||
|
||
# %% | ||
# Motivation |
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.
This example is already very long, and broadcasting and decorators are hard concepts to understand.
Although the camera example is very very cool, and even topically relevant for our audience, I find that's it's too complex for this example, because we are asking readers to consider trigonometry concepts, and besides it takes much space.
My take is that since broadcasting and decorators are complex enough, every other concept in this example should be as simple as possible. So I'd suggest replacing the camera FOV calculation with sth much simpler. For example, now that we've merged the basic ROI classes, perhaps the example could be if the individuals are within a certain polygon, or if they are on the left/right side of a vertical line. I'm also open to other, simpler ideas.
If you adopt my suggestion, I'd also consider replacing the custom dataset you construct at the beginning of this example with one of our sample datasets, for example the one with the three mice that is used in "Compute and visualise kinematics" example. Reasoning is similar: the code for creating the dataset takes up lots of space and delays getting to the essence of this example.
I'm in general worried that long examples will scare away readers. I'm aware some of our existing examples are similarly long, but I don't want to add to this problem.
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.
For example, now that we've merged the basic ROI classes, perhaps the example could be if the individuals are within a certain polygon, or if they are on the left/right side of a vertical line. I'm also open to other, simpler ideas.
Yeah you've hit the nail on the head as to why my original PR scope-drifted! 😂 I can defo re-write the example though - I'll probably use "points inside a square" as an example. The RoI
class can make an appearance at the end (since this decorator is how we're intending to write the RoI methods!)
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.
Sounds good to me!
Co-authored-by: Niko Sirmpilatze <[email protected]>
for more information, see https://pre-commit.ci
|
Description
What is this PR
Why is this PR needed?
The
shapely
library does not offer vectorised functions. The data that we will typically be seeing is 2-dimensional in space, but may have multiple other dimensions for time, individuals, etc.shapely
is not vectorised, so cannot handling broadcasting a single operation across the corresponding dimension of anumpy
array. Furthermore,shapely
also largely depends on casting to it's own internalGeometry
objects before such methods can be applied.Thus we would benefit greatly from having the ability to broadcast these 1D-functions we would need to write across dimensions automatically.
What does this PR do?
make_broadcastable
decorator into theutils
folder. More on this decorator below.make_broadcastable
is a decorator that is designed to help reduce the code bloat that "vectorising" theshapely
operations would need. It turns functions that act on 1D data (notably, theshapely
functions we will be using) into functions that can act along a given axis of aDataArray
input. More broadly,make_broadcastable
is also quite useful to have available publicly to users; they can write functions that assume they're only operating on one "piece" of data (EG one spatial coordinate) and can then use the decorators to extend this in the manner described above.As an example, suppose we have a function
is_in_unit_square
that takes a 1D (x, y) coordinate array as its input, and returns a boolean value indicating whether or not (x, y) is inside the unit square:If we now have a
DataArray
of shape (10, 2, 3) and dims ("time", "space", "individuals"),we would have to apply
is_in_unit_square
30 (=10 * 3) times to compute whether each spatial coordinate inda
was inside the unit square.To avoid having to do this in a for loop, or to re-write
is_in_unit_square
to accommodateDataArray
inputs, we can simply decorate it withmake_broadcastable
:and now we can call
was_in_unit_square
is a (10,3)DataArray
of boolean values - each entry is the result of applyingis_in_unit_square
along the corresponding"space"
dimension.make_broadcastable
also works for functions that output 1D numpy arrays too - in this case, the output replaces the dimension to be acted along (EG"space"
) with a new dimension in the outputDataArray
that contains the result.References
Tangentially related to #377, as this makes our lives a lot easier when interacting with
shapely
.How has this PR been tested?
Addition of local test suite.
The example workbook also runs without errors, which confirms the syntax is correctly employed too.
Is this a breaking change?
No
Does this PR require an update to the documentation?
An example has been added to the gallery, although it might be more tailored for use in the developer docs rather than the showcase. Happy to take opinions on this.
Checklist: