Skip to content

Weighted incidence#303

Merged
ntamas merged 9 commits intoigraph:masterfrom
PuneethaPai:weighted_incidence
Jun 29, 2020
Merged

Weighted incidence#303
ntamas merged 9 commits intoigraph:masterfrom
PuneethaPai:weighted_incidence

Conversation

@PuneethaPai
Copy link
Copy Markdown
Contributor

@PuneethaPai PuneethaPai commented Jun 21, 2020 •
edited
Loading

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
Copy Markdown
Contributor Author

PuneethaPai commented Jun 21, 2020

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
Copy Markdown
Contributor Author

PuneethaPai commented Jun 21, 2020

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
Copy Markdown
Contributor Author

PuneethaPai commented Jun 21, 2020

@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

ntamas
ntamas requested changes Jun 26, 2020
View reviewed changes
Copy link
Copy Markdown
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.

@PuneethaPai
Copy link
Copy Markdown
Contributor Author

PuneethaPai commented Jun 28, 2020

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

@ntamas
Copy link
Copy Markdown
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
@jboynyc
use weighted incidence support in igraph
5be89eb 
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

Reviewers

@ntamas ntamas ntamas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PuneethaPai @ntamas