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

Weighted incidence #303

Merged
merged 9 commits into from
Jun 29, 2020
Merged

Weighted incidence #303

merged 9 commits into from
Jun 29, 2020

Conversation

PuneethaPai
Copy link
Contributor

@PuneethaPai PuneethaPai commented Jun 21, 2020

Fixes Issue: #229

I have handled following cases w.r.t permutations of params:

  • Allow empty str as weighted attr name
  • Raise ValueError when multiple and weighted can not co-exist.
  • weighted=True graph becomes weighted with edge_attr=weight
  • weighted="some_str" graph is un-weighted with edge_attr=some_str
  • directed=True, mode="out", weighted=True graph is weighted, direction out is respcted
  • directed=True, mode="in", weighted=True graph is weighted, direction in is respcted
  • directed=True, mode="all", weighted=True graph is weighted, direction all is respcted

@PuneethaPai
Copy link
Contributor Author

Hi @ntamas ,
In test code I have used:

  • all(list([bool])) instead of bool and bool and ...
  • I have also used self.assertListEquals(list1, list2) instead of self.assertTrue(list1 == list2)

I did not modify other parts of code.

Now to maintain homogeneity:

  • If you liked this I can apply the same to other parts of code.
  • If you do not like this I can change it back to how you all are following.

@PuneethaPai
Copy link
Contributor Author

I think i haven't handled negative weight scenario. I will refer to behavior w.r.t R implementation and accordingly handle in python.
Let me know if you have any thoughts how to tackle negative weight.
Thanks

@PuneethaPai
Copy link
Contributor Author

@ntamas
Just checked with R-igraph. It allows negative values to the weights.
So I think we are good. I was in assumption that we would not allow negative weights.
You can merge this. Let me know if there are any cases I have missed or any improvements in code.

Thanks

Copy link
Member

@ntamas ntamas left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! I have requested a few minor changes, but otherwise it seems good to go. I'll merge this when the requests are addressed.

src/igraph/__init__.py Outdated Show resolved Hide resolved
src/igraph/__init__.py Outdated Show resolved Hide resolved
src/igraph/__init__.py Show resolved Hide resolved
src/igraph/__init__.py Outdated Show resolved Hide resolved
@PuneethaPai
Copy link
Contributor Author

@ntamas why is wheel build is not happening in win32 x86 system? How to debug this issue?

@ntamas
Copy link
Member

ntamas commented Jun 29, 2020

This is probably an issue with the C core and is unrelated to your PR. I'll merge this and then fix the failing build in a separate commit. Thanks for your contribution!

@ntamas ntamas merged commit 22df25d into igraph:master Jun 29, 2020
@PuneethaPai PuneethaPai deleted the weighted_incidence branch June 29, 2020 15:07
jboynyc added a commit to jboynyc/textnets that referenced this pull request Nov 10, 2022
I requested it and it was added YEARS ago:
igraph/python-igraph#303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants