adding CCC

[mhajij]

adding CCC tutorial

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[mhajij]

@ninamiolane I think I addressed that issue, but there seems still to be differences between my local env and this one. Tests on my local env are passing.

[ninamiolane]

Tests need to pass on github. This seems to be a problem with mypy: you can click on "Details" next to the failing tests to see the exact logs.

[ffl096]

Please don't use typing.List anymore, it's deprecated. Use the built-in list.

[ffl096]

The value None is invalid for a parameter of type str.

Suggested change

	update_func: str = None,
	update_func: str | None = None,

[ffl096]

You are returning two tensors. Please remember that tuple is special to all other collections in that you specifically annotate each individual element of the tuple. See https://docs.python.org/3/library/typing.html#annotating-tuples

Suggested change

	) -> tuple[torch.Tensor]:
	) -> tuple[torch.Tensor, torch.Tensor]:

[ffl096]

Suggested change

	) -> tuple[torch.Tensor]:
	) -> tuple[torch.Tensor, torch.Tensor]:

[ffl096]

According to the types, t_to_s_attention and neighbourhood_t_to_s are tensors, which do not have a values function.

[ManuelLecha]

Totally right! Suggested to change types to torch.sparse.FloatTensor in attention method.

[ffl096]

Suggested change

	update_func: str = None,
	update_func: str | None = None,

[ffl096]

What about if none of the cases match?

[ninamiolane]

+1

also elif is not needed after a return statement. You can use if.

[ffl096]

Please don't overwrite a variable with another type, just use a different name. This is hard to read.

[mhajij]

@rballeba this is confusing for me too. I tried to fix this but I am not sure what you are trying to do here in your implementation. Can you please fix this PR from here? this is supposed to be easy for you since you coded this layer originally. Thanks!

[ManuelLecha]

Suggested change

from typing import List

[ManuelLecha]

Suggested change

	def update(
	self, message_on_source: torch.Tensor, message_on_target: torch.Tensor
	) -> tuple[torch.Tensor]:
	def update(
	self, message_on_source: torch.Tensor, message_on_target: torch.Tensor
	) -> tuple[torch.FloatTensor, torch.FloatTensor]:

[ManuelLecha]

Suggested change

	def attention(
	self, s_message: torch.Tensor, t_message: torch.Tensor
	) -> tuple[torch.Tensor]:
	def attention(
	self, s_message: torch.Tensor, t_message: torch.Tensor
	) -> tuple[torch.sparse.FloatTensor, torch.sparse.FloatTensor]:

[ManuelLecha]

Suggested change

	def forward(
	self, x_source: torch.Tensor, x_target: torch.Tensor, neighborhood: torch.Tensor
	) -> tuple[torch.Tensor]:
	def forward(
	self, x_source: torch.Tensor, x_target: torch.Tensor, neighborhood: torch.Tensor
	) -> tuple[torch.FloatTensor, torch.FloatTensor]:

[ManuelLecha]

Totally right! Suggested to change types to torch.sparse.FloatTensor in attention method.

[ManuelLecha]

Suggested change

	elif self.update_func == "relu":
	return torch.nn.functional.relu(message)
	elif self.update_func == "tanh":
	return torch.nn.functional.tanh(message)
	if self.update_func == "relu":
	return torch.nn.functional.relu(message)
	if self.update_func == "tanh":
	return torch.nn.functional.tanh(message)
	else:
	raise RuntimeError(
	"Update function not recognized. Should be either sigmoid, "
	"relu or tanh."
	)

[ManuelLecha]

Suggested change

	) -> torch.Tensor:
	) -> torch.sparse.FloatTensor:

[ManuelLecha]

Suggested change

	) -> torch.Tensor:
	) -> torch.FloatTensor:

[ManuelLecha]

Suggested change

	result = torch.eye(x_source.shape[0], device=self.get_device()).to_sparse_coo()
	neighborhood = [
	result := torch.sparse.mm(neighborhood, result) for _ in range(self.m_hop)
	]
	att = [
	self.attention(m_p, A_p, a_p)
	for m_p, A_p, a_p in zip(message, neighborhood, self.att_weight)
	]
	def sparse_hadamard(A_p, att_p):
	return torch.sparse_coo_tensor(
	indices=A_p.indices(),
	values=att_p.values() * A_p.values(),
	size=A_p.shape,
	device=self.get_device(),
	)
	neighborhood = [
	sparse_hadamard(A_p, att_p) for A_p, att_p in zip(neighborhood, att)
	]
	message = [torch.mm(n_p, m_p) for n_p, m_p in zip(neighborhood, message)]
	result = torch.eye(x_source.shape[0], device=self.get_device()).to_sparse_coo()
	neighborhood = [
	result := torch.sparse.mm(neighborhood, result) for _ in range(self.m_hop)
	]
	att = [
	self.attention(m_p, A_p, a_p)
	for m_p, A_p, a_p in zip(message, neighborhood, self.att_weight)
	]
	def sparse_hadamard(A_p, att_p):
	return torch.sparse_coo_tensor(
	indices=A_p.indices(),
	values=att_p.values() * A_p.values(),
	size=A_p.shape,
	device=self.get_device(),
	)
	neighborhood = [
	sparse_hadamard(A_p, att_p) for A_p, att_p in zip(neighborhood, att)
	]
	message = [torch.mm(n_p, m_p) for n_p, m_p in zip(neighborhood, message)]

[ManuelLecha]

Suggested change

	update_func: str = None,
	update_func: str | None = None

[mhajij]

@ManuelLecha I cannot approve this PR before all tests pass.

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e6be4f1) 96.36% compared to head (0261791) 96.54%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   96.36%   96.54%   +0.18%     
==========================================
  Files          55       58       +3     
  Lines        2036     2230     +194     
==========================================
+ Hits         1962     2153     +191     
- Misses         74       77       +3     

Files	Coverage Δ
topomodelx/nn/combinatorial/hmc_layer.py	100.00% <100.00%> (ø)
topomodelx/nn/combinatorial/hmc.py	88.88% <88.88%> (ø)

... and 29 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.