Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shihchengli
Copy link
Contributor

Description

Address #1107

Checklist

  • linted with flake8?
  • (if appropriate) unit tests added?

@shihchengli shihchengli marked this pull request as ready for review November 19, 2024 16:59
@shihchengli shihchengli added the bug Something isn't working label Nov 19, 2024
@jjacobgreen
Copy link

Looks good to me. Have tested against my failing minimal reproducible example and it's fixed the issue.

@jjacobgreen
Copy link

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?

@KnathanM KnathanM self-requested a review November 20, 2024 00:58
@shihchengli
Copy link
Contributor Author

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).

@KnathanM
Copy link
Member

@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.
In conftest we have both a normal mpnn and a dirichlet mpnn defined, both without an unscale transform in the FFN.

@pytest.fixture(scope="session")
def mpnn(request):
message_passing, agg = request.param
ffn = nn.RegressionFFN()
return models.MPNN(message_passing, agg, ffn, True)
@pytest.fixture(scope="session")
def classification_mpnn_dirichlet(request):
agg = nn.SumAggregation()
ffn = nn.BinaryDirichletFFN()
return models.MPNN(request.param, agg, ffn, True)

We use the classification_mpnn_dirichlet in integration/test_classification_mol.py

@pytest.mark.parametrize(
"classification_mpnn_dirichlet",
[nn.BondMessagePassing(), nn.AtomMessagePassing()],
indirect=True,
)
@pytest.mark.integration
def test_dirichlet_overfit(classification_mpnn_dirichlet, dataloader):

We could do something similar and have an MVE mpnn defined in conftest.py and use it in integration/test_regression_mol.py. Then we would test if MVE works without the unscale transform and the CLI tests will still test if MVE works with an unscale transform.

Copy link
Member

@KnathanM KnathanM left a 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 = [
Copy link
Member

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.

@shihchengli shihchengli requested a review from KnathanM December 23, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v2 BUG]: Can't train with MveFFN or Evidential FFN without UnscaleTransform
3 participants