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

Dist2Cycle never calls Dist2CycleLayers #218

Open
2 tasks
ffl096 opened this issue Sep 22, 2023 · 5 comments
Open
2 tasks

Dist2Cycle never calls Dist2CycleLayers #218

ffl096 opened this issue Sep 22, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@ffl096
Copy link
Member

ffl096 commented Sep 22, 2023

While Dist2CycleLayers are correctly constructed in Dist2Cyle, they are never actually called.

  • Correctly call layers in the forward method.
  • This should have been catched by tests. Fix them!
@ffl096 ffl096 added the bug Something isn't working label Sep 22, 2023
@mhajij
Copy link
Member

mhajij commented Oct 6, 2023

what is the update on this issue @maneelusf ?

@maneelusf
Copy link
Contributor

Hi @mhajij , I have looked at the code and it looks like a simple calling of the Distlayer does not resolve things. Something on the below lines did not fix it.

from topomodelx.nn.simplicial.dist2cycle_layer import Dist2CycleLayer
for _ in range(n_layers):
            layers.append(
                Dist2CycleLayer(
                    channels=channels,
                )
            )
for layer in layers:
    x_1e = layer(x_1e,incidence_0,adjacency_0)

This means that we have to go through the Dist2Cycle paper once again and look through its implementation(https://github.com/alexdkeros/Dist2Cycle) to understand the changes.

@ninamiolane
Copy link
Collaborator

@maneelusf any update on this?

@maneelusf
Copy link
Contributor

Hi @ninamiolane , did not have the time to work on this last month due to other commitments. This bug requires a deep dive into the original implementation.(https://github.com/alexdkeros/Dist2Cycle). Will work on this and keep you updated on this by this Monday.

@gbg141
Copy link
Member

gbg141 commented Nov 4, 2023

Hey! Working on the tutorial checks I realized there was a bug in Dist2CycleLayer when defining the dimensions of the self.fc_neigh. I made Dist2Cycle work with multiple layers by changing the output dimension of self.fc_neigh to be compatible with chanels dim.

However, even though now the model can be executed without errors (currently in the simplicial_checks branch), I agree that it would be nice to double check the implementation of this model.

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

No branches or pull requests

5 participants