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

Add reading and writing for graphs in DIMACS graph format #4591

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

CharJon
Copy link
Contributor

@CharJon CharJon commented Feb 3, 2021

Closes #4590
See here for details about the format.

Mohammed Ghannam and others added 30 commits August 7, 2020 12:29
* Simple randomized maxcut heuristic

* Remove approximation guarantee test

Test is not expected to pass all the time due to randomness.

* Preserve random state for cut heuristic

* Add one exchange max cut heuristic

* Fixes && Refactoring && Default deterministic output

* maxcut/simple.py: Add docstring and make initial_cut optional.

* maxcut/simple.py: Add shuffle again. Make initial set a set.

Co-authored-by: Jonas Charfreitag <jcharfreitag@cs.uni-bonn.de>
* Fix parse_edgelist behavior with multiple attributes

* fixed test case

Co-authored-by: chris <chris@orchid>
* updated draw_networkx + added test

* added newline

* skip test if numpy is not installed

* change skip if numpy is not available

* switch elif to if
* add code for tree isomorphism, with tests

* fix typo in comment

* one more typo in comments

* fix some PEP8 formatting, that flake8 didn't care about

* rename files as tree_isomorphism

* run code through black formatter

* incorporate feedback from dschult in PR4067

* fix missing import for not_implemented_for decorator

* swap edge order randomly in testing routine positive_single_tree

* run black on test_tree_isomorphism.py

* spacing tweak to allow CI test of docs

Co-authored-by: Dan Schult <dschult@colgate.edu>
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I haven't looked very carefully yet but some quick thoughts/tips.

  • connect the new module's docs to the document reference in /doc/reference/readwrite/index.rst as well as a new file dimacs.rst to mimic the other readwrite sections.
  • The doc_string of generate_dimacs doesn't match the function signature... others?
  • The tests only use the node attribute "value". What about edge attributes and/or other attribute names?
  • I don't understand the meaning of a line like p 3 3 in the dimacs file. I could/will look it up, but is there an easy way to describe what it does? Would it be worth a 1 paragraph description of p # #, n # #, e # #? Or are the subtleties that make it worth having users read the format doc from the start?

@CharJon
Copy link
Contributor Author

CharJon commented Feb 4, 2021

Thanks for this!

I haven't looked very carefully yet but some quick thoughts/tips.

  • connect the new module's docs to the document reference in /doc/reference/readwrite/index.rst as well as a new file dimacs.rst to mimic the other readwrite sections.
  • The doc_string of generate_dimacs doesn't match the function signature... others?
  • The tests only use the node attribute "value". What about edge attributes and/or other attribute names?
  • I don't understand the meaning of a line like p 3 3 in the dimacs file. I could/will look it up, but is there an easy way to describe what it does? Would it be worth a 1 paragraph description of p # #, n # #, e # #? Or are the subtleties that make it worth having users read the format doc from the start?

Thanks for your feedback.
The docs and doc_strings should be updated / fixed.
The core DIMACS format only allows node attributes called value.
The p-line contains meta information about the graph (number of nodes and number of edges). I added a comment to the code. Let me know if we should add more comments / anything else.

@rossbar rossbar self-requested a review February 25, 2021 08:21
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I only did a quick pass but a few things jumped out. I'm not familiar with the DIMACS format and though some material is referenced in the module docstring, I think it would be an improvement to have (if possible) a brief description of the format there as well.

Format
------
The DIMACS graph format is a human readable text format. See
http://archive.dimacs.rutgers.edu/pub/challenge/graph/doc/ccformat.dvi
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a link to a hosted page instead of a downloadable .dvi file? If not NBD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most comprehensive one I could find.

networkx/readwrite/dimacs.py Outdated Show resolved Hide resolved
Comment on lines 70 to 75
assert (
expected_num_nodes == g.number_of_nodes()
), f"Expected to read {expected_num_nodes} nodes but found {g.number_of_nodes()}"
assert (
expected_num_edges == g.number_of_edges()
), f"Expected to read {expected_num_edges} edges but found {g.number_of_edges()}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think asserts should be avoided code, due to differences in behavior under certain conditions. This does seem a more specialized case than a ValueError though, since it's a final validation check before returning.

In this case, I think I would define a new exception for this module (say class DimacsValidationError(Exception)) and raise that on failure. Maybe other reviewers would have better suggestions though... please chime in!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an excpetion.

use nx.tempty_graph

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Base automatically changed from master to main March 4, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants