-
Notifications
You must be signed in to change notification settings - Fork 596
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
Not use transform_variance
for unscaled targets
#1108
base: main
Are you sure you want to change the base?
Conversation
Looks good to me. Have tested against my failing minimal reproducible example and it's fixed the issue. |
I don't know how feasible it is, but adding an extra test case with a predictor that has uncertainty built in might have caught this issue? |
We already have test cases for MVE and evidential methods (see here as an example). To capture the issue you found, we would need to test regression tasks without target normalization. AFAIK, we don't have a flag to disable target scaling, since the target should always be scaled to ensure stable model training (we also scale targets in the test you refer to). |
@shihchengli We scale the targets in the test he refers to, but we don't use the scaler from that to make an unscale transform for the FFN. chemprop/tests/integration/conftest.py Lines 11 to 24 in f0ee198
We use the chemprop/tests/integration/test_classification_mol.py Lines 100 to 106 in f0ee198
We could do something similar and have an MVE mpnn defined in |
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'll merge after we remove the commented out lines. @shihchengli if you aren't able to do this but agree with removing them, just say so and I can push that change.
), | ||
pytest.mark.integration, | ||
] | ||
# pytestmark = [ |
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 think we can remove these lines instead of commenting them out.
Description
Address #1107
Checklist