Change approach to input and output paths. Issue 47 by greebie · Pull Request #63 · archivesunleashed/graphpass

greebie · 2018-09-20T20:11:18Z

GitHub issue(s):

What does this Pull Request do?

accept ./graphpass {input path} {output path} -flags --optionA {data}
input path and output path can be anywhere in command line, provided that input comes first and output comes second (other non-option args are ignored for now).
include helper functions get_directory and get_filename.
issue in comments of Implement stdin stdout procedures addressing Issue #47. #62 regarding directories being created has NOT been addressed in this PR.

How should this be tested?

Should pass Travis.
Should work in ubuntu (linux) and Mac.
Tests should pass.
./graphpass {/path/to/file.graphml} {/path/to/output} -q should do a quickpass to output.graphml.
./graphpass {/path/to/file.graphml} {/path/to/output/} -qgshould do a quickpass to file.gexf

Additional Notes:

./graphpass {/path/to/file.graphml} {/path/to/} -q may replace the input file file.graphml in current commits. This will need to be repaired or managed (see comments in #62).

Interested parties

@ianmilligan1 @ruebot

Thanks in advance for your help with the Archives Unleashed Project!

- --input (-i) and --output (-o) can also be used. - functions have been created to extract directory and filename from path files. - will require additional unit testing of code.

…nux).

ianmilligan1 · 2018-09-24T22:09:19Z

Thanks for re-staging the PR @greebie. Looking good! Two quick queries.

First – just tested on an Ubuntu machine with syntax like:

./graphpass /mnt/vol1/data_sets/auk_datasets/graphpass_test/9717-gephi.graphml ./9717-gephi.gexf -qg

Seems to be working but this popped up as an error before SUCCESS:

non-option ARGV-elements: OUTARG:./9717-gephi.gexf

Is this expected behaviour?

Second, if I don't provide a relative path but just the filename, i.e.

./graphpass /mnt/vol1/data_sets/auk_datasets/graphpass_test/9717-gephi.graphml 9717-gephi2.gexf -qg

I get

non-option ARGV-elements: OUTARG:9717-gephi2.gexf
Segmentation fault (core dumped)

Is this expected behaviour?

ianmilligan1 · 2018-09-24T22:21:16Z

Also, circling back to the outstanding comments on #62. @ruebot noted:

It will only try to create the output directory if stdout does not exist. Otherwise it will just use stdout.

That's not how stdout works. We shouldn't be clobbering an existing file if an option isn't given, nor creating a new directory.

and

Can you explain the rationale here? It's my understanding that stdout should just be stdout. I don't understand the use case of forcing the creation of an output dir.

Is it possible to move forward on making this straightforward stdin stdout syntax? Thanks @greebie!

greebie · 2018-09-25T00:57:29Z

First – just tested on an Ubuntu machine with syntax like:
./graphpass /mnt/vol1/data_sets/auk_datasets/graphpass_test/9717-gephi.graphml ./9717-gephi.gexf -qg
Seems to be working but this popped up as an error before SUCCESS:
non-option ARGV-elements: OUTARG:./9717-gephi.gexf

This is the same as the previous behavior, but I would like to change it.

Is this expected behaviour?

Second, if I don't provide a relative path but just the filename, i.e.
./graphpass /mnt/vol1/data_sets/auk_datasets/graphpass_test/9717-gephi.graphml 9717-gephi2.gexf -qg
I get
non-option ARGV-elements: OUTARG:9717-gephi2.gexf
Segmentation fault (core dumped)
Is this expected behaviour?

This is not the expected behavior. I will fix.

greebie · 2018-09-25T20:44:21Z

Also, circling back to the outstanding comments on #62. @ruebot noted:

It will only try to create the output directory if stdout does not exist. Otherwise it will just use stdout.

That's not how stdout works. We shouldn't be clobbering an existing file if an option isn't given, nor creating a new directory.

and

Can you explain the rationale here? It's my understanding that stdout should just be stdout. I don't understand the use case of forcing the creation of an output dir.

Is it possible to move forward on making this straightforward stdin stdout syntax? Thanks @greebie!

I think I have this resolved in the current push. However, in doing so I broke a test (as per Travis fail).

I would also like to create some tests (tomorrow) before this merges.

ianmilligan1 · 2018-09-25T20:47:14Z

OK, can you ping me @greebie when you want me to test this. I'm out of office tomorrow afternoon and Thursday morning so might be a delay.

- get_directory - get_file - strip_ext - load_graph - write_graph

…f exit(1). - Exit should be handled in main().

greebie · 2018-09-26T18:27:11Z

Assuming the latest build passes Travis, it should complete all the outstanding issues mentioned.

graphpass does not create a new directory, other than for unit tests (which are deleted after build).
will not create an out file if stdout and stdin paths are the same.
will work for test case where stdout is just a file, defaulting to "./".
created test suite for the new functions, get_directory and get_filename to try and capture issues before they hatch.

ianmilligan1

Thanks @greebie. Looking good – I've just tested and built on Mac and Ubuntu. One outstanding problem still left, I think, from @ruebot:

We shouldn't be clobbering an existing file if an option isn't given

Right now it overwrites the file. Is there a way to stop that unless there's an override?

ruebot

First pass review. There will be more to come.

First, please use stderr instead of manually creating errors.

ruebot · 2018-09-28T12:55:42Z

@@ -48,7 +80,7 @@ extern int load_graph (char* filename) {
  fp = fopen(filename, "r");
  if (fp == 0) {
    printf(">>> FAILURE - Could not find graphML file at filepath location.\n");


Use sterr here.

ruebot · 2018-09-28T12:55:50Z

-  if (stat(ug_OUTPUT, &st) == -1) {
-    mkdir(ug_OUTPUT, 0700);
+  if (stat(ug_OUTPATH, &st) == -1) {
+    printf(">>> FAILURE - Could not create file at selected output location.\n");


ruebot · 2018-09-28T12:56:09Z

+      }
    } else {
-      igraph_write_graph_graphml(graph, fp, 1);
+      printf ("\n ERROR: Output path %s could not be accessed. Graphpass", ug_OUTPATH);


- change printf("..") to fprintf(stderr, "..") - change stderror_vector to std_error_vector. This is not stderr but a calculation of standard error for a t-test.

ianmilligan1 · 2018-09-28T22:24:30Z

@greebie since you pinged me to say you weren't able to reproduce the overwriting files.

On rho if I run:

./graphpass /mnt/vol1/data_sets/auk_datasets/graphpass_test/9717-gephi.graphml 9717-gephi2.gexf -qg

It overwrites the existing file.

greebie · 2018-10-01T13:24:41Z

Hi Ian - when you say it overwrites the existing file, do you mean 9719-gephi2.gexf or 9717-gephi.graphml? I was worried about the latter and the former would (currently) be the expected behavior. I hadn't considered the possibility of preventing over-write if it's not desired.

I can add an over-write flag and make it so that 9717-gephi2.gexf becomes 9717-gephi.gexf0 (or 9717-gephi.gexf1 and so on) if that's the desired behavior you are looking for?

ianmilligan1 · 2018-10-01T13:29:06Z

I can add an over-write flag and make it so that 9717-gephi2.gexf becomes 9717-gephi.gexf0 (or 9717-gephi.gexf1 and so on) if that's the desired behavior you are looking for?

That sounds good to me. What do you think @ruebot ?

greebie · 2018-10-01T13:36:28Z

Okay - is it okay if we put that in another issue? It's possible that I can break things while building this functionality, so I'd like to be able to track it.

ruebot · 2018-10-01T16:58:55Z

@greebie I'm noticing FAILURE messages in the build, but the tests don't fail. Is this expected behaviour? I see it here, and locally.

[nruest@wombat:graphpass] (git)-[issue-47]-$ make clean all
rm -f qp
rm -f ana
rm -f io
rm -rf TEST_OUT_FOLDER
rm -rf build/
rm -f graphpass
gcc ./vendor/unity/unity.c ./src/tests/runner_test_qp.c -I./src/headers -I/usr/local/include/igraph -I./vendor/unity ./src/tests/quickrun_test.c src/main/analyze.c src/main/filter.c src/main/gexf.c src/main/io.c src/main/quickrun.c src/main/reports.c src/main/rnd.c src/main/viz.c -L/usr/local/lib -ligraph -lm -o qp
gcc ./vendor/unity/unity.c ./src/tests/runner_test_ana.c -I./src/headers -I/usr/local/include/igraph -I./vendor/unity ./src/tests/analyze_test.c src/main/analyze.c src/main/filter.c src/main/gexf.c src/main/io.c src/main/quickrun.c src/main/reports.c src/main/rnd.c src/main/viz.c -L/usr/local/lib -ligraph -lm -o ana
gcc ./vendor/unity/unity.c ./src/tests/runner_test_io.c -I./src/headers -I/usr/local/include/igraph -I./vendor/unity ./src/tests/io_test.c src/main/analyze.c src/main/filter.c src/main/gexf.c src/main/io.c src/main/quickrun.c src/main/reports.c src/main/rnd.c src/main/viz.c -L/usr/local/lib -ligraph -lm -o io
./ana
src/tests/analyze_test.c:14:TEST_DEGREE_ALGORITHM:PASS
src/tests/analyze_test.c:25:TEST_INDEGREE_ALGORITHM:PASS
src/tests/analyze_test.c:36:TEST_OUTDEGREE_ALGORITHM:PASS
src/tests/analyze_test.c:47:TEST_BETWEENNESS_ALGORITHM:PASS
src/tests/analyze_test.c:58:TEST_AUTHORITY_ALGORITHM:PASS
src/tests/analyze_test.c:69:TEST_HUB_ALGORITHM:PASS
src/tests/analyze_test.c:99:TEST_EIGENVECTOR_ALGORITHM:IGNORE: Eigenvector has some random elements that need to be seeded
src/tests/analyze_test.c:91:TEST_PAGERANK_ALGORITHM:PASS
src/tests/analyze_test.c:102:TEST_MODULARITY:PASS
src/tests/analyze_test.c:113:TEST_RANKORDER:PASS
src/tests/analyze_test.c:138:TEST_MEAN:PASS
src/tests/analyze_test.c:151:TEST_VARIANCE:PASS
src/tests/analyze_test.c:164:TEST_STD:PASS
src/tests/analyze_test.c:177:TEST_STDERROR:PASS
src/tests/analyze_test.c:190:TEST_TTEST:PASS
src/tests/analyze_test.c:203:TEST_TPVALUE:PASS

-----------------------
16 Tests 0 Failures 1 Ignored 
OK
./qp
src/tests/quickrun_test.c:22:TEST_QUICKRUN_NOSAVE:PASS
src/tests/quickrun_test.c:29:TEST_QUICKRUN_DEGREE:PASS
src/tests/quickrun_test.c:51:TEST_QUICKRUN_COLOR:PASS
src/tests/quickrun_test.c:40:TEST_QUICKRUN_SIZE:PASS
src/tests/quickrun_test.c:72:TEST_QUICKRUN_GEXF:PASS
src/tests/quickrun_test.c:80:TEST_QUICKRUN_GRAPHML:PASS

-----------------------
6 Tests 0 Failures 0 Ignored 
OK
./io
src/tests/io_test.c:43:TEST_GET_DIRECTORY:PASS
src/tests/io_test.c:55:TEST_GET_FILE:PASS
src/tests/io_test.c:68:TEST_STRIP_EXT:PASS
>>> FAILURE - Could not find graphML file at filepath location.
src/tests/io_test.c:74:TEST_LOAD_GRAPH:PASS
>>> FAILURE - Could not create file at selected output location.
>>>         - Ensure that your assigned output folder exists.
src/tests/io_test.c:85:TEST_WRITE_GRAPH:PASS

-----------------------
5 Tests 0 Failures 0 Ignored 
OK
gcc src/main/*.c -I./src/headers -I/usr/local/include/igraph -I./vendor/unity -L/usr/local/lib -ligraph -lm -o graphpass
./graphpass -qnv
>>>>>>>  GRAPHPASSING >>>>>>>> 
FILEPATH: src/resources/cpp2.graphml
OUTPUT DIRECTORY: ./
PERCENTAGE: 0.000000
FILE: cpp2.graphml
METHODS STRING: d
QUICKRUN: 1
REPORT: 0
SAVE: 0
Running graphpass on file: src/resources/cpp2.graphml
Successfully ingested graph with 218 nodes and 220 edges.


Quickrun requested.

Quickrun does no filtering, and provides layout information
based on Degree (nodesize), Walktrap Modularity (color), and
the Fructerman-Rheingold algorithm to maximize space between
nodes.

Quickrun is quicker, but less informative in terms of output.
Calculating Degree... 
Calculating Modularity... 
Assigning Colors... 
Assigning Degree values... 
Scaling graph with degree values... 
Producing layout details... 


>>>>  SUCCESS!- NO_SAVE requested, so no output.

You'll also need to update .gitignore to include the new items to the build process:

TEST_OUT_FOLDER/
ana
io
qp

greebie · 2018-10-01T17:02:49Z

@ruebot That is the expected behavior because I created tests that check that graphpass fails when it is supposed to, but I do not think this is desired behavior. Let me see if I can test those features more gracefully.

ruebot · 2018-10-01T17:07:14Z

@ianmilligan1 with the overwrite. Can you add the steps you took? If the gexf file already exists and the output location, it is going get clobbered. That's how cat or any utility is going to work.

ianmilligan1 · 2018-10-01T17:18:05Z

I'm happy with the overwriting if you are, @ruebot, as I was just noting as you said above "If the gexf file already exists and the output location, it is going get clobbered." If we're cool with that happy to move forward.

ruebot · 2018-10-01T17:27:33Z

Ah, yeah. I see what you mean. I was talking about the OUTPUT directory getting created, and everything that went with that. Sorry for the confusion there.

greebie · 2018-10-01T17:55:34Z

Okay great. The current issues I will address are:

add new test files to .gitIgnore
try to avoid fail messages on tests, probably by creating a TEST flag.

Do not print error messages on tests. Add ana, io, & qp to .gitignore

greebie · 2018-10-02T13:49:11Z

Latest commit shows this travis result:

And confirm failure shows on regular run.

ruebot

Functionality appears to be good to go. Lots of cleanup here, and after that I believe we should be good to go.

Add some comments to help demystify PATH management process. (This may need re-factoring at a later time.)

ruebot

Couple tiny things, then I think we're good to go.

greebie added 3 commits September 20, 2018 14:52

Change input method in preparation for stdin stdout approach.

9ad7c88

- --input (-i) and --output (-o) can also be used. - functions have been created to extract directory and filename from path files. - will require additional unit testing of code.

Update README.md to reflect current approach to command line utility.

af15fbd

Remove references to libgen in Makefile (no longer used for Mac or Li…

b7f6745

…nux).

greebie added 2 commits September 25, 2018 15:41

Resolve issue discovered in PR testing.

e57cddb

FAIL when filepath and output path are the same to avoid overwriting.

3cd7921

greebie added 5 commits September 25, 2018 16:50

Fix test error.

ee9637d

Add test suite for input-output functions.

88e4ee2

- get_directory - get_file - strip_ext - load_graph - write_graph

Remove debug flags from tests and have functions return (1) instead o…

a84d57b

…f exit(1). - Exit should be handled in main().

Include clean before running install.

c1d1c7b

Forgot to add test files for io.

9f17647

Use #define instead of const for Ubuntu gcc.

87845f3

ianmilligan1 requested changes Sep 27, 2018

View reviewed changes

ruebot requested changes Sep 28, 2018

View reviewed changes

Use stderr instead of stdout for error outputs.

dc4255d

- change printf("..") to fprintf(stderr, "..") - change stderror_vector to std_error_vector. This is not stderr but a calculation of standard error for a t-test.

Minor test improvements from PR review.

7add94d

Do not print error messages on tests. Add ana, io, & qp to .gitignore

ruebot requested changes Oct 2, 2018

View reviewed changes

Various README and content fixes from PR.

53138d3

Add some comments to help demystify PATH management process. (This may need re-factoring at a later time.)

ruebot reviewed Oct 3, 2018

View reviewed changes

Comment thread src/headers/graphpass.h Outdated

ruebot reviewed Oct 3, 2018

View reviewed changes

Comment thread src/headers/graphpass.h Outdated

ruebot reviewed Oct 3, 2018

View reviewed changes

Comment thread src/headers/graphpass.h Outdated

Fix errant periods in macros.

c8bb08c

ianmilligan1 approved these changes Oct 3, 2018

View reviewed changes

ruebot requested changes Oct 4, 2018

View reviewed changes

Comment thread src/main/reports.c Outdated

Comment thread src/headers/graphpass.h Outdated

Comment thread src/headers/graphpass.h Outdated

Documentation tweaks as per PR review.

fb05bfa

ruebot approved these changes Oct 4, 2018

View reviewed changes

ruebot merged commit 1d9c65b into master Oct 4, 2018

ruebot deleted the issue-47 branch October 4, 2018 19:43

Conversation

greebie commented Sep 20, 2018

What does this Pull Request do?

How should this be tested?

Additional Notes:

Interested parties

Uh oh!

ianmilligan1 commented Sep 24, 2018

Uh oh!

ianmilligan1 commented Sep 24, 2018

Uh oh!

greebie commented Sep 25, 2018

Uh oh!

greebie commented Sep 25, 2018

Uh oh!

ianmilligan1 commented Sep 25, 2018

Uh oh!

greebie commented Sep 26, 2018

Uh oh!

ianmilligan1 left a comment

Choose a reason for hiding this comment

Uh oh!

ruebot left a comment

Choose a reason for hiding this comment

Uh oh!

ruebot Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

ruebot Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

ruebot Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

ianmilligan1 commented Sep 28, 2018

Uh oh!

greebie commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianmilligan1 commented Oct 1, 2018

Uh oh!

greebie commented Oct 1, 2018

Uh oh!

ruebot commented Oct 1, 2018

Uh oh!

greebie commented Oct 1, 2018

Uh oh!

ruebot commented Oct 1, 2018

Uh oh!

ianmilligan1 commented Oct 1, 2018

Uh oh!

ruebot commented Oct 1, 2018

Uh oh!

greebie commented Oct 1, 2018

Uh oh!

greebie commented Oct 2, 2018

Uh oh!

ruebot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ruebot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

greebie commented Oct 1, 2018 •

edited

Loading