[Feature] Rework `to_one_hot` and `to_categorical` to take a tensor as parameter #816

riiswa · 2023-01-10T14:30:24Z

Description

As mentioned in the closed PR #783 we want to update the methods to_onehot and to_categorical to support value conversion in addition to spec conversion.

This is what currently happens

spec = torchrl.data.MultOneHotDiscreteTensorSpec([5,10])
sample = spec.rand((2,3))
sample.shape
torch.Size([2, 3, 15])
sample_np =spec.to_numpy(sample)
sample_np.shape
(2, 3, 2)

spec = torchrl.data.MultOneHotDiscreteTensorSpec([5,10])
sample = spec.rand((2,3))
sample.shape
torch.Size([2, 3, 15])
sample_np =spec.to_numpy(sample)
sample_np.shape
(2, 3, 15)

sample_cat = spec.to_categorical(sample)
sample_cat.shape
torch.Size([2, 3, 2])

To keep the current behaviour (conversion of the spec), I decided that val will be an optional parameter. The documentation will be well written to explain the different behaviours of theses methods.

Motivation and Context

Also related to the issue #781

I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

New feature (non-breaking change which adds core functionality)
Documentation (update in the documentation)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

I have read the CONTRIBUTION guide (required)
My change requires a change to the documentation.
I have updated the tests accordingly (required for a bug fix or a new feature).
I have updated the documentation accordingly.

vmoens · 2023-01-19T11:56:12Z

Hey @riiswa can I help to unblock this?

riiswa · 2023-01-19T12:46:24Z

Hey @riiswa can I help to unblock this?

Hey, I'm a little bit busy in this moment (exams at school). I think the PR is finished, I juste have a little bug on GPU (created tensor are not on the right device) 😄, and I don't have gpu to test locally.

I think you can start the PR review, and when I have time I'll fix this little thing (unless you want to do it yourself)

vmoens · 2023-01-20T10:00:30Z

Np i'll have a look

…ategorical methods

riiswa · 2023-02-16T06:58:48Z

@vmoens I'm finally back, and I think this PR is ready for a review😄
@matteobettini maybe you should also check, because it's your idea

vmoens

Sorry to shuffle things a little: don't we want two separate methods for these?

like to_one_hot and DiscreteTensorSpec.to(OneHotDiscreteTensorSpec)?

vmoens · 2023-02-16T10:00:13Z

torchrl/data/tensor_specs.py

+    def to_categorical(
+        self, val: Optional[torch.Tensor] = None, safe: bool = True
+    ) -> Union[DiscreteTensorSpec, torch.Tensor]:
+        """Convert a given one-hot tensor in categorical format or convert the spec to the equivalent categorical spec.


Suggested change

"""Convert a given one-hot tensor in categorical format or convert the spec to the equivalent categorical spec.

"""Converts a given one-hot tensor in categorical format or converts the spec to the equivalent categorical spec.

vmoens · 2023-02-16T10:00:24Z

torchrl/data/tensor_specs.py

+    def to_categorical(
+        self, val: Optional[torch.Tensor] = None, safe: bool = True
+    ) -> Union[MultiDiscreteTensorSpec, torch.Tensor]:
+        """Convert a given one-hot tensor in categorical format or convert the spec to the equivalent categorical spec.


Suggested change

"""Convert a given one-hot tensor in categorical format or convert the spec to the equivalent categorical spec.

"""Converts a given one-hot tensor in categorical format or converts the spec to the equivalent categorical spec.

vmoens · 2023-02-16T10:02:10Z

torchrl/data/tensor_specs.py

+    def to_onehot(
+        self, val: Optional[torch.Tensor] = None, safe: bool = True
+    ) -> Union[OneHotDiscreteTensorSpec, torch.Tensor]:
+        """One-hot encode a given tensor or convert the spec to the equivalent one-hot spec.


Suggested change

"""One-hot encode a given tensor or convert the spec to the equivalent one-hot spec.

"""Encodes a discrete tensor from the spec domain into its one-hot correspondent, or converts the spec to the equivalent one-hot spec.

vmoens · 2023-02-16T10:02:35Z

torchrl/data/tensor_specs.py

+    def to_onehot(
+        self, val: Optional[torch.Tensor] = None, safe: bool = True
+    ) -> Union[MultiOneHotDiscreteTensorSpec, torch.Tensor]:
+        """One-hot encode a given tensor or convert the spec to the equivalent one-hot spec.


Suggested change

"""One-hot encode a given tensor or convert the spec to the equivalent one-hot spec.

"""Encodes a discrete tensor from the spec domain into its one-hot correspondent, or converts the spec to the equivalent one-hot spec.

riiswa · 2023-02-16T18:29:42Z

Yes it's cleaner, but in case of DiscreteTensorSpec.to(OneHotDiscreteTensorSpec) what can be the use of the parameter? I mean we will always convert it to the same OneHotDiscreteTensorSpec. The method can be named to_one_hot_spec()

vmoens · 2023-02-16T20:55:51Z

Yes it's cleaner, but in case of DiscreteTensorSpec.to(OneHotDiscreteTensorSpec) what can be the use of the parameter? I mean we will always convert it to the same OneHotDiscreteTensorSpec. The method can be named to_one_hot_spec()

My idea (but happy to challenge it) was that some day in the future we may code -- say -- a HashDiscreteSpec where a hash key is used to code a discrete value (or some other embedding).
Then it may be useful to have a generic to(SpecType) method that handles all sorts of conversion.

We could also map Bounded to Unbounded with the same structure, etc.

By I agree that to_one_hot_spec is more explicit.

riiswa · 2023-02-17T08:33:05Z

Then it may be useful to have a generic to(SpecType) method that handles all sorts of conversion.

For the moment I have written a specific function, let's work on the generic version in the future after implementing the HashDiscreteSpec!

vmoens

LGTM thanks for this!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 10, 2023

riiswa changed the title ~~[Feature] Rework to_onehot and to_categorical to take a tensor as parameter (WIP)~~ [WIP, Feature] Rework to_onehot and to_categorical to take a tensor as parameter Jan 10, 2023

riiswa force-pushed the feature/to_categorical_and_to_one_hot_method_tensor_parameter branch from 0cd5ed4 to ae10757 Compare January 13, 2023 20:38

riiswa marked this pull request as ready for review January 19, 2023 12:46

riiswa changed the title ~~[WIP, Feature] Rework to_onehot and to_categorical to take a tensor as parameter~~ [Feature] Rework to_onehot and to_categorical to take a tensor as parameter Jan 19, 2023

riiswa force-pushed the feature/to_categorical_and_to_one_hot_method_tensor_parameter branch 5 times, most recently from 63a8764 to 0736e9e Compare January 24, 2023 01:50

riiswa added 5 commits February 15, 2023 21:08

Rework MultOneHotDiscreteTensorSpec to_categorical_method

ecefb66

Rework MultiDiscreteTensorSpec to_onehot method

e05f9db

Rework OneHotDiscreteTensorSpec & DiscreteTensorSpec to_onehot & to_c…

e1eec1d

…ategorical methods

Add docs

4e6b5b5

Remove to numpy method

02a7c8e

riiswa force-pushed the feature/to_categorical_and_to_one_hot_method_tensor_parameter branch from 0736e9e to 02a7c8e Compare February 15, 2023 20:10

Fix device in MultiDiscreteTensorSpec.is_in

4b875a6

vmoens reviewed Feb 16, 2023

View reviewed changes

Separe conversion functions for spec and for tensor

5f3e311

riiswa changed the title ~~[Feature] Rework to_onehot and to_categorical to take a tensor as parameter~~ [Feature] Rework to_one_hot and to_categorical to take a tensor as parameter Feb 17, 2023

vmoens approved these changes Feb 20, 2023

View reviewed changes

vmoens merged commit 099ced3 into pytorch:main Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Rework `to_one_hot` and `to_categorical` to take a tensor as parameter #816

[Feature] Rework `to_one_hot` and `to_categorical` to take a tensor as parameter #816

riiswa commented Jan 10, 2023 •

edited

Loading

vmoens commented Jan 19, 2023

riiswa commented Jan 19, 2023 •

edited

Loading

vmoens commented Jan 20, 2023

riiswa commented Feb 16, 2023

vmoens left a comment

vmoens Feb 16, 2023

vmoens Feb 16, 2023

vmoens Feb 16, 2023

vmoens Feb 16, 2023

riiswa commented Feb 16, 2023 •

edited

Loading

vmoens commented Feb 16, 2023

riiswa commented Feb 17, 2023

vmoens left a comment

	"""Convert a given one-hot tensor in categorical format or convert the spec to the equivalent categorical spec.
	"""Converts a given one-hot tensor in categorical format or converts the spec to the equivalent categorical spec.

	"""One-hot encode a given tensor or convert the spec to the equivalent one-hot spec.
	"""Encodes a discrete tensor from the spec domain into its one-hot correspondent, or converts the spec to the equivalent one-hot spec.

[Feature] Rework to_one_hot and to_categorical to take a tensor as parameter #816

[Feature] Rework to_one_hot and to_categorical to take a tensor as parameter #816

Conversation

riiswa commented Jan 10, 2023 • edited Loading

Description

Motivation and Context

Types of changes

Checklist

vmoens commented Jan 19, 2023

riiswa commented Jan 19, 2023 • edited Loading

vmoens commented Jan 20, 2023

riiswa commented Feb 16, 2023

vmoens left a comment

Choose a reason for hiding this comment

vmoens Feb 16, 2023

Choose a reason for hiding this comment

vmoens Feb 16, 2023

Choose a reason for hiding this comment

vmoens Feb 16, 2023

Choose a reason for hiding this comment

vmoens Feb 16, 2023

Choose a reason for hiding this comment

riiswa commented Feb 16, 2023 • edited Loading

vmoens commented Feb 16, 2023

riiswa commented Feb 17, 2023

vmoens left a comment

Choose a reason for hiding this comment

[Feature] Rework `to_one_hot` and `to_categorical` to take a tensor as parameter #816

[Feature] Rework `to_one_hot` and `to_categorical` to take a tensor as parameter #816

riiswa commented Jan 10, 2023 •

edited

Loading

riiswa commented Jan 19, 2023 •

edited

Loading

riiswa commented Feb 16, 2023 •

edited

Loading