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 a convenience function to save an InputGraph to a file. #35

Closed
wants to merge 1 commit into from

Conversation

dabreegster
Copy link
Contributor

Every time I send over a sample input graph, I think I wind up writing some code like this; not sure why it was missing originally.

Also, we could just use serde and automatically serialize/deserialize input graphs to JSON, bincode, etc, instead of this custom format. But maybe that's less useful, since the internal format of InputGraph might evolve over time.

@easbar
Copy link
Owner

easbar commented Sep 9, 2021

Every time I send over a sample input graph, I think I wind up writing some code like this; not sure why it was missing originally.

Sure, why not, I think the only reason this was missing is that I did not really export any graphs.

Then again, it seems a bit inconsistent that we offer such methods to read/write InputGraph, but not for FastGraph.

Also, we could just use serde and automatically serialize/deserialize input graphs to JSON, bincode, etc, instead of this custom format

The current read method is compatible with the DIMACS format, which I think is useful. But of course we could also add an executable that converts DIMACS to our internal/serialized format and only use Serde within the library, just like we do with FastGraph. But since the file I/O code is included anyway the current read method does no harm either?!

Slightly off-topic, but to me the biggest issue with saving/loading graphs right now is that the bincode/serde serialization seems really slow? Did you notice this as well? This code takes a very long time to save a FastGraph:

   let fast_graph_file = File::create("my_graph.fp").unwrap();
   bincode::serialize_into(fast_graph_file, &fast_graph).unwrap();

InputGraph::read_from_file isn't running very fast either...

Comment on lines +206 to +208
/// Writes the input graph to a text file, using the following format:
/// a <from> <to> <weight>
/// Mostly used for performance testing.
Copy link
Owner

Choose a reason for hiding this comment

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

If we want to keep DIMACS compatibility we would have to increment from/to by one, as the nodes must be 1-based. But then of course we also need to adjust the read method accordingly.

src/input_graph.rs Outdated Show resolved Hide resolved
@easbar
Copy link
Owner

easbar commented Sep 9, 2021

Slightly off-topic, but to me the biggest issue with saving/loading graphs right now is that the bincode/serde serialization seems really slow?

If this wasn't so I'd say we could use Serde consistently for InputGraph and FastGraph and move things like a DIMACS conversion tool to a separate binary.

@dabreegster
Copy link
Contributor Author

Then again, it seems a bit inconsistent that we offer such methods to read/write InputGraph, but not for FastGraph.

FastGraph uses serde, so people can read/write it in any format they want. I'm not sure it'd be useful for us to pick a binary encoding for the main library. Also, I would guess that people aren't just saving a FastGraph; it's part of a larger structure. InputGraph is different -- the use case is more tightly scoped, using for benchmarking.

The current read method is compatible with the DIMACS format, which I think is useful. But of course we could also add an executable that converts DIMACS to our internal/serialized format

Ah, I didn't realize it's a common format -- maybe that could be more clear in the docstring. I think a separate executable to do the translation sounds heavyweight; just a library call should be fine? And there's no great need for serde for input graphs; I was just curious why there was a custom format.

the bincode/serde serialization seems really slow? Did you notice this as well?

I haven't noticed anything unusual. Running with --release and using a BufWriter are two things to double check.

@easbar
Copy link
Owner

easbar commented Sep 11, 2021

FastGraph uses serde, so people can read/write it in any format they want. I'm not sure it'd be useful for us to pick a binary encoding for the main library. Also, I would guess that people aren't just saving a FastGraph; it's part of a larger structure.

Ok, sure. Serde is definitely useful and of course we should keep it.

And there's no great need for serde for input graphs; I was just curious why there was a custom format.

I just noticed InputGraph already has the Serialize/Deserialize traits as well. But I think the read method is useful anyway, because it allows importing a graph from a text file and does not add any dependencies.

Ah, I didn't realize it's a common format -- maybe that could be more clear in the docstring.

Well, I just used this format because it is so simple (just one line per edge and comments are possible). The only difference is that the DIMACS format is 1-based and we use 0-based node IDs internally. This does not make a difference as long as we are only reading, but when we add a write function I think we should change this. Otherwise our write function will produce files that are not compatible with DIMACS. I'll open a separate PR that changes the format. -> #36

I haven't noticed anything unusual. Running with --release and using a BufWriter are two things to double check.

Ok, thanks. I was already using the --release flag, but I will do some measurements and open a separate issue in case I still find it to be too slow.

@easbar
Copy link
Owner

easbar commented Sep 11, 2021

There are now separate functions to read/write the previous text format and the dimacs format: 3d77616

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.

None yet

2 participants