-
Notifications
You must be signed in to change notification settings - Fork 118
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
[FEAT] Add .image.crop Expression #1175
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1175 +/- ##
==========================================
+ Coverage 88.42% 88.45% +0.03%
==========================================
Files 54 54
Lines 5555 5562 +7
==========================================
+ Hits 4912 4920 +8
+ Misses 643 642 -1
|
src/daft-core/src/array/ops/image.rs
Outdated
DaftImageBuffer::LA(image_buffer_vec_to_cow(result)) | ||
} | ||
DaftImageBuffer::RGB(imgbuf) => { | ||
let imgbuf_copy = image_buffer_cow_to_vec(imgbuf); |
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.
Took me a few hours to figure out the lifetimes here, but I ended up making a copy of the Cow into a Vec on the stack.
A naive imgbuf.clone()
didn't seem to work -- for some reason the Cow's lifetime (tied to &self
) was making the crop_imm
function very unhappy.
Not sure why, so this is my workaround. I believe 2 copies are being made here - once in image_buffer_cow_to_vec
and once in .to_image()
which materializes the crop.
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 was really hoping to not have to materialize any intermediates until after the crop and then do a .to_image()
. However, the error message I get if I naively pass in imgbuf
without copying the Cow into a Vec is:
borrowed data escapes outside of method
`self` escapes the method body hererustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20%5B0%5D?0#file%3A%2F%2F%2FUsers%2Fjaychia%2Fcode%2FDaft%2Fsrc%2Fdaft-core%2Fsrc%2Farray%2Fops%2Fimage.rs)
image.rs(227, 17): `self` is a reference that is only valid in the method body
image.rs(120, 6): lifetime `'a` defined here
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.
Hacked around this issue: ab0b606
Adds an expression for cropping images based on a column of bounding boxes