Simplify scalers, move to `gluonts.torch.scaler` #2632

lostella · 2023-02-09T12:58:41Z

Description of changes: There's no need for scalers to be torch.nn.Module since they don't really hold parameters. Also fixes the default keepdim of MeanScaler for consistency.

cc @kashif

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup

kashif · 2023-02-09T13:00:02Z

very cool! LGTM!

jaheba · 2023-02-09T13:00:32Z

src/gluonts/torch/scaler.py

@@ -37,21 +36,12 @@ class MeanScaler(nn.Module):
        minimum possible scale that is used for any item.
    """

-    @validated()


We loose the validation property?

Should I use our own dataclass?

Yes, let's give it a try

I'll just keep them validated for now

jaheba · 2023-02-09T13:04:00Z

src/gluonts/torch/scaler.py

+    default_scale: float = 0.0
+    minimum_scale: float = 1e-10


I think these need to be torch.tensor

torch.clamp also deals with numbers; for the default_scale, isn't this some kind of "immutable" (mind the quotes) property of the object, so that replacing torch.where with if is not really harmful?

shubhamkapoor · 2023-02-09T13:18:10Z

src/gluonts/torch/scaler.py

-        self.register_buffer("minimum_scale", torch.tensor(minimum_scale))
+    dim: int = -1
+    keepdim: bool = False
+    minimum_scale: float = 1e-5


Also, minimum_scale should be a torch.Tensor.

Why? I think we can add a torch.Tensor to a number

lostella · 2023-02-09T14:14:28Z

~~It's funny that all tests pass on macOS~~

jaheba · 2023-02-09T17:39:40Z

src/gluonts/torch/scaler.py

-            self.default_scale,
-            batch_scale,
-        )
+        if self.default_scale is not None:


What is the effect if this change on tracing?

I think that, as long as self.default_scale stays constant (which it should) then tracing should be fine with it and produce the right code for the model. But I need to verify

Also according to the warning here https://pytorch.org/docs/stable/generated/torch.jit.trace.html#torch-jit-trace it should be fine as long as the control flow is not affected by the value in input tensors (and I would add: if it’s not otherwise changed throughout the model execution, like the forward changing the value of self.default_scale here for some reason)

abdulfatir · 2023-02-10T14:15:25Z

Sorry for the late comment but any takers for Scalers being of torch.distributions.transforms.Transform type, AffineTransform in particular. This provides us with the inverse function that you can just apply back to your scaled output and also provides log_abs_det_jacobian, if needed for loss computation.

kashif · 2023-02-10T14:19:13Z

@abdulfatir on the output side the distribution is an AffineTransform which takes the loc and scale from the scalers... but you mean more general scalers? And I believe currently it's an appropriate place to have the scalers (in the model) since then the model can use the loc and scale as input as well (instead of just on the emission side).

abdulfatir · 2023-02-10T14:31:37Z

Currently, I don't have an example of a general scaler in mind, but yes, using torch.distributions.transforms.Transform can provide that flexibility. Regarding access of scaler params (loc, scale) in the models, they can also be accessed from the scaler/transform object's properties.

The primary benefit IMO is clarity. You have a scaler (Transform) that normalizes the data and then use its inverse (and log_abs_det_jacobian) at the output side.
@kashif

abdulfatir · 2023-02-10T14:34:30Z

Also, inside models I think we should be able to provide a Scaler() object instead of a boolean scaling as currently implemented.

lostella · 2023-02-13T13:04:46Z

@abdulfatir I think it makes sense to consider this. The (log) scale should be accessible via log_abs_det_jacobian if one needs that as input to the model, but not the location. I’ll give it a try and share how that goes

abdulfatir · 2023-02-13T13:18:48Z

@lostella If the transform is of AffineTransform type, then we can access the .loc and .scale attributes. How a model handles/uses transforms could be their internal matter. For instance, for DeepAR like models we can restrict the scale to be of AffineTransform type and freely access these properties.

abdulfatir · 2023-02-13T13:26:56Z

For completeness, I would like to add that the inverse transform (which we would need at the output side) is available via the .inv attribute.

import torch
from torch.distributions.transforms import AffineTransform

tr = AffineTransform(10., 1.)
inv_tr = tr.inv

x = torch.rand(2, 3, 4)
y = tr(x)

torch.allclose(x, inv_tr(y), atol=1e-5) # True

lostella · 2023-02-13T13:30:58Z

@lostella If the transform is of AffineTransform type, then we can access the .loc and .scale attributes. How a model handles/uses transforms could be their internal matter. For instance, for DeepAR like models we can restrict the scale to be of AffineTransform type and freely access these properties.

The catch is that these scaling operations are not really affine transformations of the data: for some array x, x and 2*x get scaled down to the same vector. They are once you fix the loc and scale, so you can access them as properties of the transformation, but scale (and possibly loc) depend on the input data here.

lostella · 2023-02-16T13:02:32Z

Also, inside models I think we should be able to provide a Scaler() object instead of a boolean scaling as currently implemented.

@abdulfatir agreed! We can do that in a separate PR

lostella requested a review from shubhamkapoor February 9, 2023 12:58

lostella added the BREAKING This is a breaking change (one of pr required labels) label Feb 9, 2023

lostella changed the title ~~Turn torch scalers into callable dataclasses, move to gluonts.torch.scaler~~ Make torch scalers callable dataclasses, move to gluonts.torch.scaler Feb 9, 2023

jaheba reviewed Feb 9, 2023

View reviewed changes

shubhamkapoor reviewed Feb 9, 2023

View reviewed changes

lostella added the torch This concerns the PyTorch side of GluonTS label Feb 9, 2023

jaheba reviewed Feb 9, 2023

View reviewed changes

lostella force-pushed the torch-scalers-dataclasses branch from 7bbe5be to 25921da Compare February 10, 2023 09:25

Lorenzo Stella added 6 commits February 10, 2023 14:26

turn scalers into callable objects

6bf8c97

fixup

002b410

simplify comment

cc4c027

fix after rebase

725ab29

use our own dataclass

5ba2c63

switch back to validated, fix default keepdim

d5bcf75

lostella force-pushed the torch-scalers-dataclasses branch from 364c701 to d5bcf75 Compare February 10, 2023 13:26

lostella requested review from jaheba and shubhamkapoor February 10, 2023 13:49

lostella changed the title ~~Make torch scalers callable dataclasses, move to gluonts.torch.scaler~~ Make torch scalers callable objects, move to gluonts.torch.scaler Feb 10, 2023

lostella changed the title ~~Make torch scalers callable objects, move to gluonts.torch.scaler~~ Simplify scalers, move to gluonts.torch.scaler Feb 10, 2023

jaheba approved these changes Feb 10, 2023

View reviewed changes

lostella added 2 commits February 16, 2023 13:38

Merge branch 'dev' into torch-scalers-dataclasses

df5df12

Merge branch 'dev' into torch-scalers-dataclasses

0b63f59

lostella enabled auto-merge (squash) February 16, 2023 13:02

lostella merged commit c5b64b4 into awslabs:dev Feb 16, 2023

lostella deleted the torch-scalers-dataclasses branch May 23, 2023 20:02

lostella mentioned this pull request May 23, 2023

Fix default scale in torch DeepAR #2885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify scalers, move to `gluonts.torch.scaler` #2632

Simplify scalers, move to `gluonts.torch.scaler` #2632

lostella commented Feb 9, 2023 •

edited

Loading

kashif commented Feb 9, 2023

jaheba Feb 9, 2023

lostella Feb 9, 2023

jaheba Feb 9, 2023

lostella Feb 10, 2023

jaheba Feb 9, 2023

lostella Feb 9, 2023

shubhamkapoor Feb 9, 2023

lostella Feb 9, 2023

lostella commented Feb 9, 2023 •

edited

Loading

jaheba Feb 9, 2023

lostella Feb 9, 2023 •

edited

Loading

lostella Feb 9, 2023

abdulfatir commented Feb 10, 2023

kashif commented Feb 10, 2023 •

edited

Loading

abdulfatir commented Feb 10, 2023 •

edited

Loading

abdulfatir commented Feb 10, 2023

lostella commented Feb 13, 2023

abdulfatir commented Feb 13, 2023

abdulfatir commented Feb 13, 2023

lostella commented Feb 13, 2023 •

edited

Loading

lostella commented Feb 16, 2023

Simplify scalers, move to gluonts.torch.scaler #2632

Simplify scalers, move to gluonts.torch.scaler #2632

Conversation

lostella commented Feb 9, 2023 • edited Loading

kashif commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostella commented Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

lostella Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulfatir commented Feb 10, 2023

kashif commented Feb 10, 2023 • edited Loading

abdulfatir commented Feb 10, 2023 • edited Loading

abdulfatir commented Feb 10, 2023

lostella commented Feb 13, 2023

abdulfatir commented Feb 13, 2023

abdulfatir commented Feb 13, 2023

lostella commented Feb 13, 2023 • edited Loading

lostella commented Feb 16, 2023

Simplify scalers, move to `gluonts.torch.scaler` #2632

Simplify scalers, move to `gluonts.torch.scaler` #2632

lostella commented Feb 9, 2023 •

edited

Loading

lostella commented Feb 9, 2023 •

edited

Loading

lostella Feb 9, 2023 •

edited

Loading

kashif commented Feb 10, 2023 •

edited

Loading

abdulfatir commented Feb 10, 2023 •

edited

Loading

lostella commented Feb 13, 2023 •

edited

Loading