giotto-ai / pyflagser

Python bindings and API for the flagser C++ library (https://github.com/luetge/flagser).
Other
13 stars 14 forks source link

`save_weighted_flag` crashes (and is uncovered by tests) #58

Closed flomlo closed 3 years ago

flomlo commented 3 years ago

Description

save_weighted_flag throws error instead of doing it's job.

Steps/Code to Reproduce

1) Have a binary adjacency matrix G 2) call save_weighted_flag('foo.bar', G)

Expected Results

saves flag to foo.bar

Actual Results

crashes:

Traceback (most recent call last):
  File "/home/florian/project/digraph-analyzer/playground.py", line 14, in <module>
    save_unweighted_flag('/tmp/bbp_1.flag', G)
  File "/home/florian/.local/lib/python3.9/site-packages/pyflagser/flagio.py", line 196, in save_unweighted_flag
    np.savetxt(f, edges, comments='', header='dim 1', fmt='%i %i')
  File "<__array_function__ internals>", line 5, in savetxt
  File "/usr/lib/python3.9/site-packages/numpy/lib/npyio.py", line 1404, in savetxt
    raise error
ValueError: fmt has wrong number of % formats:  %i %i

Versions

everything is fresh from the git

Fix:

The following diff fixes this:

[florian@frankenstein-archlinux pyflagser]$ git diff
diff --git a/pyflagser/flagio.py b/pyflagser/flagio.py
index 7f3ebc9..446bf17 100644
--- a/pyflagser/flagio.py
+++ b/pyflagser/flagio.py
@@ -191,9 +191,8 @@ def save_unweighted_flag(fname, adjacency_matrix):
     vertices, edges = _extract_unweighted_graph(adjacency_matrix)

     with open(fname, 'w') as f:
-        np.savetxt(f, vertices, delimiter=' ', comments='', header='dim 0',
-                   fmt='%.18e')
-        np.savetxt(f, edges, comments='', header='dim 1', fmt='%i %i')
+        np.savetxt(f, vertices.reshape((1,-1)), delimiter=' ', header='dim 0', fmt='%i')
+        np.savetxt(f, edges, comments='', header='dim 1', fmt='%i %i %i')

 def save_weighted_flag(fname, adjacency_matrix, max_edge_weight=None):

I'll open a PR soon. First need to figure out how :D

ulupo commented 3 years ago

Thanks for spotting this! Looks like it should be easy to fix, thanks for volunteering! I agree that tests should be added, would you like to have a go at that too?

flomlo commented 3 years ago

I've never written test suits before, but I can just copy-paste the code beloging to save_weighted_flag, which is covered by the test suit iirc.

ulupo commented 3 years ago

Fixed by #60