Faith pd cli #70
Faith pd cli #70
Conversation
|
Seems like there is some conflict due to the updated requirements that is causing the compile step to fail. Maybe coming from Cython? I do not experience this issue on Mac |
|
@ElDeveloper @wasade re: #71 seem reasonable? |
|
That is very reasonable! It is both QIIME1 and QIIME2 compatible, so we should be good to go for the most part. |
sucpp/api.hpp
Outdated
| * The following error codes are returned: | ||
| * | ||
| * write_okay : no problems | ||
| * open_error : could not open the file |
There was a problem hiding this comment.
It doesn't look like this error is actually returned, am I missing something?
There was a problem hiding this comment.
yeah good catch, copypasta again. Open error is used to prevent opening multiple files at one time when writing partials. since we aren't supporting parallel execution at this point, we do not need it. I've removed this line.
There was a problem hiding this comment.
Furthermore, it is not returned by write_mat so I have removed it there as well.
| const std::string &tree_filename = input.getCmdOption("-t"); | ||
| const std::string &output_filename = input.getCmdOption("-o"); | ||
|
|
||
| faith_cli_one_off(table_filename, tree_filename, output_filename); |
There was a problem hiding this comment.
This line should be:
return faith_cli_one_off(table_filename, tree_filename, output_filename);
And also, now you should be able to remove line 82.
There was a problem hiding this comment.
I think this is fine as is. Error handling is taken care of here:
https://github.com/gwarmstrong/unifrac/blob/f6ac379ecf80625ed2ab91211d71f2e80e44c9a9/sucpp/faithpd.cpp#L57-L66
And follows the convention previously set in sucpp/su.cpp
| const std::string &tree_filename = input.getCmdOption("-t"); | ||
| const std::string &output_filename = input.getCmdOption("-o"); | ||
|
|
||
| faith_cli_one_off(table_filename, tree_filename, output_filename); |
|
Thanks! |
Resolves #66 and resolves #69
Introduces a cli for Faith's PD with the following signature:
This PR should probably wait for #68 so I can include the osx compatibility for the Faith PD cli compatibility for mac in this PR.