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

Fix bug in Collector with Graph data #456

Merged
merged 2 commits into from
Feb 20, 2025
Merged

Fix bug in Collector with Graph data #456

merged 2 commits into from
Feb 20, 2025

Conversation

FilippoOlivo
Copy link
Member

This PR fix some bugs with solvers and Graphs

@FilippoOlivo FilippoOlivo force-pushed the 0.2Data branch 4 times, most recently from f32d7e4 to 6f52c79 Compare February 18, 2025 10:36
@FilippoOlivo FilippoOlivo marked this pull request as ready for review February 18, 2025 10:36
@FilippoOlivo FilippoOlivo requested a review from ndem0 February 18, 2025 10:36
@FilippoOlivo FilippoOlivo self-assigned this Feb 18, 2025
@FilippoOlivo FilippoOlivo added the pr-to-review Label for PR that are ready to been reviewed label Feb 18, 2025
@FilippoOlivo FilippoOlivo force-pushed the 0.2Data branch 9 times, most recently from 6c66cf6 to f32af3c Compare February 19, 2025 12:00
pina/utils.py Outdated
@@ -48,7 +48,8 @@ def labelize_forward(forward, input_variables, output_variables):
:type output_variables: list[str] | tuple[str]
"""
def wrapper(x):
x = x.extract(input_variables)
if isinstance(x, LabelTensor):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this? This method should not check the instance since it is applied only when use_lt is true in the solver. Please remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use graphs x is a Batch object. It is necessary if I want to use graphs and LabelTensors together

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't see the point. In the solver do you set use_lt to true?

@dario-coscia dario-coscia added pr-to-fix Label for PR that needs modification and removed pr-to-review Label for PR that are ready to been reviewed labels Feb 19, 2025
pina/utils.py Outdated
@@ -48,7 +48,8 @@ def labelize_forward(forward, input_variables, output_variables):
:type output_variables: list[str] | tuple[str]
"""
def wrapper(x):
x = x.extract(input_variables)
if isinstance(x, LabelTensor):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't see the point. In the solver do you set use_lt to true?

@dario-coscia
Copy link
Collaborator

To me ok! Maybe @ndem0 let's first merge #457 and then this one because I think less conflicts will be created by doing this

@FilippoOlivo FilippoOlivo added the pr-to-review Label for PR that are ready to been reviewed label Feb 20, 2025
@FilippoOlivo FilippoOlivo added v0.2 implementation in v0.2 and removed pr-to-fix Label for PR that needs modification labels Feb 20, 2025
@dario-coscia dario-coscia merged commit 669d870 into 0.2 Feb 20, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-to-review Label for PR that are ready to been reviewed v0.2 implementation in v0.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants