Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ten/five crop augmentation #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Junxi-Chen
Copy link

Add ten/five crop augmentation when extract the I3D features. Solve issue #92 #72 . And add save_option to the i3d.yaml file to save only reg features. Because I think that fps and timestamp features are really redundant. The shape of the rgb features imply timestamp.

Thank you for your great work. 馃殌馃殌

Copy link
Owner

@v-iashin v-iashin left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request. However, it should be significantly revised before merging.

I'd be more positive about it if:

  1. one pull request wouldn't implement two features: one for the ten crop, the other for the changes in disk writing
  2. I don't like that each crop is treated as a separate video (now each video creates 10/5 times more files. can't we implement crop as a batch dimension?
  3. the augs are applied only to RGB stream, not both.
  4. the augs are implemented for i3d but not other models.
  5. i am not happy that the classes/functions that are common for many models are being changed without reflecting on how logic for other features that depend on them changes

i appreciate the efforts but i am not convinced that it is enough

self.i3d_transforms = {
'rgb': torchvision.transforms.Compose([
TensorCenterCrop(self.central_crop_size),
aug_transform,
Copy link
Owner

Choose a reason for hiding this comment

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

any reason why we can't do it for the flow?



if self.aug_type is not None:
feats_dict = {stream: [[] for _ in range(self.num_crop)] for stream in self.streams}
Copy link
Owner

Choose a reason for hiding this comment

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

Why treat each crop as a separate tensor instead of a batch dimension: B, Crops, D --> B*Crops, D?

@@ -145,13 +145,17 @@ def __call__(self, tensor: torch.FloatTensor) -> torch.FloatTensor:

class ScaleTo1_1(object):

def __call__(self, tensor: torch.FloatTensor) -> torch.FloatTensor:
def __call__(self, tensor):
if isinstance(tensor, tuple):
Copy link
Owner

Choose a reason for hiding this comment

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

lost typing

return (2 * tensor / 255) - 1


class PermuteAndUnsqueeze(object):

def __call__(self, tensor: torch.FloatTensor) -> torch.FloatTensor:
def __call__(self, tensor):
if isinstance(tensor, tuple):
Copy link
Owner

Choose a reason for hiding this comment

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

lost typing

@@ -50,9 +50,18 @@ def show_predictions_on_dataset(logits: torch.FloatTensor, dataset: Union[str, L
print(f'{logit:8.3f} | {smax:.3f} | {cls}')
print()

def make_path(output_root, video_path, output_key, ext):
def make_path(output_root, video_path, output_key, ext, idx=None):
Copy link
Owner

Choose a reason for hiding this comment

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

we shouldn't resort to this. it became incredibly redundant. we need to save all features in one file

@@ -76,6 +78,11 @@ def action_on_extraction(
return

for key, value in feats_dict.items():
if self.save_option == 'rgb_only':
Copy link
Owner

Choose a reason for hiding this comment

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

what's wrong with the streams argument in i3d?

This was referenced May 2, 2024
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.

None yet

2 participants