pyt-team / TopoModelX

Topological Deep Learning
https://pyt-team.github.io/topomodelx/
MIT License
234 stars 82 forks source link

`Dist2Cycle` never calls `Dist2CycleLayer`s #218

Open ffl096 opened 1 year ago

ffl096 commented 1 year ago

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

mhajij commented 1 year ago

what is the update on this issue @maneelusf ?

maneelusf commented 1 year ago

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 commented 1 year ago

@maneelusf any update on this?

maneelusf commented 1 year ago

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 commented 12 months ago

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.