Skip to content

Fuyu processing: handle coordinates#113

Merged
amyeroberts merged 13 commits into
amyeroberts:fuyu-processing-updatefrom
pcuenca:fuyu-processing-update-coordinates
Nov 1, 2023
Merged

Fuyu processing: handle coordinates#113
amyeroberts merged 13 commits into
amyeroberts:fuyu-processing-updatefrom
pcuenca:fuyu-processing-update-coordinates

Conversation

@pcuenca

@pcuenca pcuenca commented Oct 30, 2023

Copy link
Copy Markdown

Comment on lines -304 to +295
def original_to_transformed_w_coords(self, original_coords):
# apply crop
cropped_coords = (
self._clamp_coords(original_coords, min_value=self.crop_left, max_value=self.crop_right) - self.crop_left
)
# apply scale
scaled_coords = self._scale_coords(cropped_coords, scale=self.scaled_w / self.original_w)
# apply pad
return scaled_coords + self.padding_left
# Simplified assuming self.crop_left = self.padding_left = 0
def original_to_transformed_w_coords(original_coords, scale_w):
return np.round(original_coords * scale_w).astype(np.int32)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seq = tokens_to_boxes(seq, size)
seq = tokens_to_points(seq, size)
seq = seq[None, :]
# TODO: what if sequence lengths vary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested this yet, but if we use batch inference and there are different number of encoded boxes or points in the sequences, the tensor shapes will be different after these transformations. How can we deal with this case?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's best. I can see two ways: either we have a a way of padding the sequences to the max length then concatenating, or we return a list of variable length tensors.

If I've understood the method correctly, this is postprocessing the model outputs and not producing the raw outputs which would be fed back to the model when generating the next token. In this case, do we need to return a batched tensor?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a list of variable lengths tensors is fine, there's few things after generation that'd leverage a padded version of it. Just something like

results = []
for seq, size in zip(outputs, target_sizes):
    seq = tokens_to_boxes(seq, size)
    seq = tokens_to_points(seq, size)
    results.append(seq)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are both right, thanks! I've checked in other post-processors that there's no need to keep the same input type if it's not necessary.

@pcuenca

pcuenca commented Oct 30, 2023

Copy link
Copy Markdown
Author

@amyeroberts @molbap could you please take a first look?

There's a TODO about batch handling when coordinates are translated that I'm not sure how to resolve. Other than that, I'll check for potentially unused coordinate handling code that was brought over from the original repo.

@amyeroberts amyeroberts left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 🔥

seq = tokens_to_boxes(seq, size)
seq = tokens_to_points(seq, size)
seq = seq[None, :]
# TODO: what if sequence lengths vary?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's best. I can see two ways: either we have a a way of padding the sequences to the max length then concatenating, or we return a list of variable length tensors.

If I've understood the method correctly, this is postprocessing the model outputs and not producing the raw outputs which would be fed back to the model when generating the next token. In this case, do we need to return a batched tensor?

Comment thread src/transformers/models/fuyu/processing_fuyu.py Outdated
@amyeroberts
amyeroberts merged commit c523215 into amyeroberts:fuyu-processing-update Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants