-
Notifications
You must be signed in to change notification settings - Fork 688
[ENH] Refactor metrics to be polymorphic #1897
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
base: main
Are you sure you want to change the base?
Conversation
FYI @agobbifbk, @fkiraly , @PranavBhatP |
Very hard to debug, a lot of overhead for just computing the loss :-) @fkiraly did you write it or do you know the author? Not sure to be able to say something useful here, sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am deeply uncomfortable with all the case distinctions here.
This is very difficult to parse.
What we need to work with:
- proper docstrings. Do not just say the output is a
torch.Tensor
, but be clear about dimension, size, and type. To not say the output isy_pred
without any type etc. Same everywhere - ensure that is also tested reliably. Test all changed and newly introduced methods with individual input/output pairs.
If we do not do this, we risk introducing a lot of bugs and brittleness. The code would be impossible to maintain in the current state.
Minor comment: It is easier to document clearly if you stick to the numpydoc format for docstring that we use in sktime.
I still need to add docstrings, for now, I just copied the docstrings of original methods, I will update them with numpydoc format and add clear dimensions
I tested the metrics using test_metrics, and all the tests passed, but |
""" | ||
Convert network prediction into a point prediction. | ||
|
||
Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this docstring should be clarified - assumed input type, guaranteed output type
def to_quantiles( | ||
self, y_pred: torch.Tensor, quantiles: list[float] = None | ||
) -> torch.Tensor: | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this docstring should be clarified - assumed input type, guaranteed output type
@@ -320,6 +370,35 @@ def __len__(self) -> int: | |||
""" | |||
return len(self.metrics) | |||
|
|||
def _prepare_y_pred( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this docstring should be clarified - assumed input type, guaranteed output type
def to_prediction(self, y_pred: torch.Tensor, n_samples: int = 100) -> torch.Tensor: | ||
def _to_prediction_3d( | ||
self, y_pred: torch.Tensor, n_samples: int = 100 | ||
) -> torch.Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this docstring should be clarified - assumed input type, guaranteed output type
I understand that this is already how it was - which is part of the reason, perhaps, that the code base is a bit confusing. Because we are now changing this - and working in a group of people - I think we need to be super precise as to what the contracts are. We are in a really central and crucial location of the package API, I think only clean documentation (from the start) and systematic testing ensures that we do not introduce problems that are hard to spot later. |
Hi, I tried to debug the errors, I was able to do so for most part but for these params in pytorch-forecasting/pytorch_forecasting/models/deepar/_deepar_pkg.py Lines 93 to 99 in 33041c7
I am a little clueless :
dict(
loss=ImplicitQuantileNetworkDistributionLoss(hidden_size=8, input_size=98),
), but what I am more concerned about is why this is not required in the current implementation?
it is from this line : pytorch-forecasting/pytorch_forecasting/metrics/distributions.py Lines 543 to 547 in 33041c7
at first i thought this is because the default value of Edit: I ran the debugger again but now with more brakpoints, i found that at first (for most of the
dict(
loss=MultivariateNormalDistributionLoss(),
data_loader_kwargs=dict(
target_normalizer=GroupNormalizer(
groups=["agency", "sku"], transformation="log1p"
)
),
trainer_kwargs=dict(accelerator="cpu"),
), I am not sure why it is not working without normalisation this is the error:
so the normalisation makes it compatible, i understand that, but why not without it? and how my reshaping the inputs affecting things to become "not positive-definite" |
I have added the docstrings, please see if that helps or if I need to makethem more descriptive, I think I should start adding tests, maybe that can help as well... so for now I will work on tests |
I ran a quick search through co-pilot, it says that this kind of loss always requires normalization due to its existing implementation. There is no work-around to not using normalization when using this specific loss. Ig we should stick to this, not sure about the mathematical reason behind this. |
but then why it never failed before? I mean this param was added without normalisation right? and it passed at that time? then why now it fails? |
FYI, unfortunately I do not know why these parameters were chosen as they were - I just transferred them from the pre-existing tests in 1.2.0 and prior, from the |
maybe sth to discuss in a tech meeting - to give any useful input, I would need to understand the exact context of what is failing and when. |
|
||
Returns: | ||
torch.Tensor: point prediction | ||
Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good docstring!
Regarding the API: is this "as current" or "as reworked"?
- how do we ensure downwards compatibility with v1
- did we not decide on
(batch, variable, time, params)
?
This is as current, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a private method (_to_prediction_3D
) - it contains the original logic of to_prediction
that handles 2D and 3D inputs. If you look at the next function (to_prediction
) , it contains the logic to handle the (batch, time, num_target, params)
.
The new to_prediction
can handle 2D, 3D inputs (like the original implementation) as well as the 4D input.
For 2D, 3D inputs, to_prediction
uses this _to_prediction_3D
and for 4D it passes the 3D slices of [batch, time, params]
for each target
in num_target
to _to_prediction_3D
then stacks it up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this "as current" or "as reworked"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR description, can you please be more detailed of what you are doing, please?
- what is the state prior to the PR
- how are you changing it
Please be precise.
Also, a "refactor" means that public APIs remain the same - I guess this is not the case here? Or are you widening the contract?
So as discussed on discord, we decided to make the metrics polymorphic, i.e, it can handle any dimension. So if this is the case, then even the original model implementations (that has 3D outputs) are also "ideally" compatible with metrics - This is what I am trying to do. So if even the original model implementations are compatible with metrics, we are just adding the support for 4D model outputs, I thought word "refactor" would explain it well. Am I messing things up here? |
Not sure - what I am missing is a clear understanding of the status quo design, and a clear description of the target design, both including signatures of functions with expected input and output types, assumes and guarantees. I would strongly recommend to write a design proposal or we might end up talking cross purpose. For terminology:
|
This PR refactors
base_metrics
to make them polymorphic, i.e, it can handle any dimension.The current
Metrics
support the 2D ([batch, time]
) and 3D ([batch, time, params]
) inputs, This PR tries to add 4D ([batch, time, num_target, params]
) input support toMetrics
.For this we mainly try to change the base
Metric
class, so that the changes are easily propagated to the child classes.The original
to_prediction
andto_quantile
methods (which support 3D inputs) are renamed to private functions -_to_prediction_3D
and_to_quantile_3D
. The newto_prediction
andto_quantile
also add support for 4D inputs.Some other cases like
MultiHorizonMetric
,MultiLoss
etc are also changed.MultiLoss
- Adds_prepare_y_pred
to ensure y_pred is a list of tensors for mulitple types of inputs - 3D, 4D.MultiHorizonMetric
- makeupdate
polymorphicDistributionLoss
- Add 4D input support by changingto_quantile
andto_prediction
functions similar to (in terms of naming only)Metrics