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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alobugdays/241-aten-w-mergeable-properties #244

Closed
wants to merge 2 commits into from

Conversation

tflahaul
Copy link
Contributor

@tflahaul tflahaul commented Nov 17, 2022

Merge augmented tensors properties into lists instead of throwing assertion errors.
Fix issue #241

_merge_tensor's behavior is unchanged when the properties are the same.

Example when using torch.cat((a, b, c), dim=0) with a, b, c being tensors with DIFFERENT projection properties

Before:

AssertionError: Trying to merge augmented tensor with different property: projection

After:

tensor(
	projection=['kumler_bauer', 'equidistant', 'pinhole']
	normalization=minmax_sym
	pose=torch.Size([3, 4, 4])
	mask=torch.Size([3, 1, H, W])
	depth=torch.Size([3, 1, H, W])
	cam_intrinsic=torch.Size([3, 4, 4])
	cam_extrinsic=torch.Size([3, 4, 4])
	distortion=[[0.45, 0.39, 0.12], 0.25, 1.0]
        ...

Example when using torch.cat((a, b, c), dim=0) with a, b, c being tensors with SAME projection properties

Before and after (the behavior is unchanged):

tensor(
	projection=kumler_bauer
	normalization=minmax_sym
	pose=torch.Size([3, 4, 4])
	mask=torch.Size([3, 1, H, W])
	depth=torch.Size([3, 1, H, W])
	cam_intrinsic=torch.Size([3, 4, 4])
	cam_extrinsic=torch.Size([3, 4, 4])
	distortion=[0.45, 0.39, 0.12]
        ...

This pull request includes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Daria Doubine and others added 2 commits November 17, 2022 09:28
merge aten properties when they're different
@tflahaul tflahaul added enhancement New feature or request aloscene aloscene labels Nov 17, 2022
@jsalotti jsalotti self-requested a review November 17, 2022 08:50
@anhtu293 anhtu293 self-assigned this Nov 17, 2022
@anhtu293 anhtu293 removed their assignment Nov 17, 2022
@thibo73800 thibo73800 self-requested a review November 17, 2022 08:59
Copy link
Contributor

@thibo73800 thibo73800 left a comment

Choose a reason for hiding this comment

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

This is an important PR
@tflahaul can you give more context about the expected behavior with one or two example ?
@jsalotti you also had multiple time the same issue

@tflahaul
Copy link
Contributor Author

@thibo73800 I gave more explanations & a minimal example in issue #241

@tflahaul tflahaul changed the title thomas/aten-w-mergeable-properties 241-aten-w-mergeable-properties Nov 17, 2022
@tflahaul tflahaul changed the title 241-aten-w-mergeable-properties alobugdays/241-aten-w-mergeable-properties Nov 17, 2022
@tflahaul tflahaul requested a review from thibo73800 November 17, 2022 09:31
@thibo73800
Copy link
Contributor

thibo73800 commented Nov 17, 2022

@jsalotti @tflahaul @anhtu293

The following work because there is indeed no conflicts between the properties

  f1 = aloscene.Frame(np.zeros((3, 200, 300)))
  f2 = aloscene.Frame(np.zeros((3, 200, 300)))
  f1.baseline = 0.5
  f2.baseline = 0.5
  c = torch.cat([f1.batch(), f2.batch()], dim=0)

The following will not work. But this is an expected behavior. Otherwise we could insert hidden issue in the pipeline.

f1.baseline = 0.5
f2.baseline = 0.6
c = torch.cat([f1.batch(), f2.batch()], dim=0)

An alternative way to do could be to create a new augmented tensor (Not a default child but a one that could be create into a specific datapipeline)

 baselines_1 = aloscene.tensors.AugmentedTensor(torch.as_tensor([0.5]), names=("N",))
 baselines_2 = aloscene.tensors.AugmentedTensor(torch.as_tensor([0.6]), names=("N",))
 f1.add_child("baselines", baselines_1, align_dim=["B","T"], mergeable=True)
 f2.add_child("baselines", baselines_2, align_dim=["B","T"], mergeable=True)
 c = torch.cat([f1.batch(), f2.batch()], dim=0)

I'm don't like the ideas of simply doing a concat on the properties if they're different since it could break some other mechanism related to child and augmented tensors (slice for example will not work for properties). An alternative could be to automatically create a new child as above when trying to merge tensors with different properties. But I don't think it must be the default behavior.

By the way: a1d27cdc1628557c44bb28f031bcd11e73df896f

@tflahaul tflahaul linked an issue Nov 17, 2022 that may be closed by this pull request
@jsalotti
Copy link
Contributor

@jsalotti @tflahaul @anhtu293

The following work because there is indeed no conflicts between the properties

  f1 = aloscene.Frame(np.zeros((3, 200, 300)))
  f2 = aloscene.Frame(np.zeros((3, 200, 300)))
  f1.baseline = 0.5
  f2.baseline = 0.5
  c = torch.cat([f1.batch(), f2.batch()], dim=0)

The following will not work. But this is an expected behavior. Otherwise we could insert hidden issue in the pipeline.

f1.baseline = 0.5
f2.baseline = 0.6
c = torch.cat([f1.batch(), f2.batch()], dim=0)

An alternative way to do could be to create a new augmented tensor (Not a default child but a one that could be create into a specific datapipeline)

 baselines_1 = aloscene.tensors.AugmentedTensor(torch.as_tensor([0.5]), names=("N",))
 baselines_2 = aloscene.tensors.AugmentedTensor(torch.as_tensor([0.6]), names=("N",))
 f1.add_child("baselines", baselines_1, align_dim=["B","T"], mergeable=True)
 f2.add_child("baselines", baselines_2, align_dim=["B","T"], mergeable=True)
 c = torch.cat([f1.batch(), f2.batch()], dim=0)

I'm don't like the ideas of simply doing a concat on the properties if they're different since it could break some other mechanism related to child and augmented tensors (slice for example will not work for properties). An alternative could be to automatically create a new child as above when trying to merge tensors with different properties. But I don't think it must be the default behavior.

I agree. I specifically don't like the idea of silently merging a property that should not be merged.

@tflahaul
Copy link
Contributor Author

Can the projection and distortion properties of atens already be expressed as childs ? @jsalotti @thibo73800
Or is there some work to do to make it work

@jsalotti
Copy link
Contributor

jsalotti commented Nov 17, 2022

Can the projection and distortion properties of atens already be expressed as childs ? @jsalotti @thibo73800 Or is there some work to do to make it work

Distortion should be OK, because it is numerical. Projection is more complicated because they are string.
This could be done by replacing it by int :

Class Projection :
    PINHOLE = 0
    EQUIDISTANT = 1
    KUMLER_BAUER= 2

setting to tensor :

 projection_1 = aloscene.tensors.AugmentedTensor(torch.as_tensor([Projection.PINHOLE]), names=("N",))
 projection_2 = aloscene.tensors.AugmentedTensor(torch.as_tensor([Projection.EQUIDISTANT]), names=("N",))
 
 f1.add_child("projection", projection_1, align_dim=["B","T"], mergeable=True)
 f2.add_child("projection", projection_2, align_dim=["B","T"], mergeable=True)
 c = torch.cat([f1.batch(), f2.batch()], dim=0)

Note : this implies to break retro-compatibility with code already using the projection property. But this could be a good practice to not use string properties anymore, because they are not easily represented into tensor. @thibo73800 what do you think ?

@thibo73800
Copy link
Contributor

@jsalotti Agree, also if one want to link the text with the child we have the aloscene.Labels property that can store the name associated with each ID. Labels could be used to store projection.

@jsalotti
Copy link
Contributor

jsalotti commented Nov 17, 2022

I still think that it is a dangerous idea to merge with different values for the same property.

For example with projection, the function depth.as_point3d has different behavior based on projection property. If the user merges two tensors with different values for projection, the method as_point3d will not work for the merged tensor.

But maybe in some cases it still is absolutely necessary to merge such tensor, even if it break some other behavior ? @tflahaul could you cite one scenario where it is the case ?

@tflahaul
Copy link
Contributor Author

@jsalotti In our case we want to train a model on data with different projections & distortion coefficients. Therefore we need to create batches of tensors using these different properties.

Since we only want to train the model, functions like as_points3d can be ignored, but we could probably handle it by having a layer of abstraction on top of our projection functions.

For example, instead of doing this in as_points3d and anywhere the projection functions are used:

if projection == "kumler_bauer":  # or Projection.KUMLER_BAUER
        ...
elif projection == "equidistant":
        ...

we could wrap it in a more general "project" function, iterating over the projection property and batch dimension of merged augmented tensors:

def project(merged_aten):
	projmap = {
		"kumler_bauer": project_kumler_bauer,
		"equidistant" : project_equidistant,
		"pinhole"     : project_pinhole,
	}
	if isinstance(merged_aten.projection, str):  # no merge happened
		projmap[projection](merged_aten, merged_aten.distortion)
	elif isinstance(merged_aten.projection, list):
		for index, projection in enumerate(frame.projection):
			projmap[projection](frame[index], frame.distortion[index])

(just a workaround thought, this needs more discussion)

@jsalotti
Copy link
Contributor

jsalotti commented Nov 17, 2022

@tflahaul the idea behind your code sample seems good, but I think we will encounter some problems. These problems are caused by the difference between property and child.

Properties lack some interesting features that children have : a child is an AugmentedTensor with names for its dimensions. It can be mergeable or not, and the align_dim argument gives us flexibility on how the child should be merged. This allows others functions to operate seemlessly on AugmentedTensor whether they have batch/temporal dimension or not.

Some example following the idea of your code sample :

f1 = Frame(torch.ones((1,3,10,10)), names=("T","C","H", "W"))
f2 = f1.clone()
f1.projection = "pinhole"
f2.projection = "kumler_bauer"
f = torch.cat([f1,f2], axis=0) # concat along temporal axis
f = f.batch() # add batch dimension
print(f.projection)
print(f.names)

If we follow the idea of creating a list for properties when merging, the output will be:

['pinhole', 'kumler_bauer']
('B', 'T', 'C', 'H', 'W')

Then, the projection function would have to guess that the two items of the list correspond to the temporal dimension, which correspond to dim=1.

I think the behavior you want to produce is much closer to what a child can do, because it is an AugmentedTensor. I would suggest to encode the projection as an AugmentedTensor of int, and define it as a child instead of a property.

@thibo73800
Copy link
Contributor

@jsalotti @tflahaul I think the best is to used Labels

aloscene.Labels([0, 1, 1, 2], labels_names=["stereo", "kumer", "equidistant"], names=("N",))

@Data-Iab
Copy link
Collaborator

closing since no activity since November

@Data-Iab Data-Iab closed this Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aloscene aloscene enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mergeable properties for augmented tensors
5 participants