Skip to content

[Question] restriction of y_max #1657

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

Closed
ftmtas212 opened this issue Apr 7, 2023 · 2 comments · Fixed by #1662
Closed

[Question] restriction of y_max #1657

ftmtas212 opened this issue Apr 7, 2023 · 2 comments · Fixed by #1662
Labels

Comments

@ftmtas212
Copy link

Short Question Description

A clear single sentence question we can try to help with?

In the predict() of Class SimpleRegressionPipeline, there are restrictions on the min/max values of y.

def predict(self, X, batch_size=None):
    y = super().predict(X, batch_size=batch_size)
    y[y > (2 * self.y_max_)] = 2 * self.y_max_
    if self.y_min_ < 0:
        y[y < (2 * self.y_min_)] = 2 * self.y_min_
    elif self.y_min_ > 0:
        y[y < (0.5 * self.y_min_)] = 0.5 * self.y_min_
    return y

My question is should we also consider y_max < 0 like y_min?

def predict(self, X, batch_size=None):
    y = super().predict(X, batch_size=batch_size)
    if self.y_max > 0:
        y[y > (2 * self.y_max_)] = 2 * self.y_max_
    elif self.y_max < 0:
        y[y > (0.5 * self.y_max_)] = 0.5 * self.y_max_
    if self.y_min_ < 0:
        y[y < (2 * self.y_min_)] = 2 * self.y_min_
    elif self.y_min_ > 0:
        y[y < (0.5 * self.y_min_)] = 0.5 * self.y_min_
    return y

If I have missed anything, please let me know.
Many thanks!

@ftmtas212 ftmtas212 changed the title [Question] My Question? [Question] restriction of y_max Apr 7, 2023
@ftmtas212 ftmtas212 reopened this Apr 13, 2023
@aron-bram
Copy link
Collaborator

Hi,
thank you for pointing this out. It's a fair question. And I do agree with you on the fact that max should be capped in the way you suggested. However, this part of the code is very old, and I did not write it so I will rather ask before changing it. Maybe there has been a good empirical reason for this.

I'm really sceptical, though, so I will mark it as a bug until all doubts are cleared.

Will let you know once I found out.

@aron-bram aron-bram added the bug label Apr 17, 2023
@aron-bram
Copy link
Collaborator

According to the author, this is indeed a bug.

Great catch! Again, thank you very much for pointing it out.

I will quickly get onto fixing it. For now, I will just use your code snippet as it's a reasonable solution and definitely the quickest one.
Later, we will likely use something more robust, like a TransformedTargetRegressor, instead of manually capping like above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants