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

Argument parsing bugfix, plus writeout suppression options. #9

Closed
wants to merge 2 commits into from

Conversation

dotsdl
Copy link

@dotsdl dotsdl commented Jan 25, 2015

No description provided.

The loadOptions function parses either options fed to it or options
obtained from STDIN. It used to choose where to take options based on
whether or not any args were given to it at all. This had the problem
that if no options were given to it, it would pull options from the
calling script whether or not these were intended for it at all. This
can result in silent failure of propka.run.single().

Now, loadOptions takes a keyword argument 'commandline' that, when
False, takes its arguments from its own input. When True, it takes them
from STDIN. The run.single() function also reflect this change.
This is useful for external codes that don't need the file output, but
would like performance gains from not waiting for a file to be written
to disk each time.

For Molecular_container.__init__(), writeout=False will suppress
writeout of propka inputs. For run.single(), writeout=False will
suppress writeout of both inputs and results.
@cstein
Copy link
Member

cstein commented Jan 27, 2015

@dotsdl I just looked at the proposed changes in this pull request along with your initial discussion about suppresion of output in issue #10 and I have a few comments and questions I'd like to discuss with the maintainers (I suppose @charnley is the one right now).

The suppression of output, I feel, is a great addition that could be interesting to do in circomstances like yours - especially if you are truly limited by the output of propka. But since it would be a major undertaking (and you are the first to request this suppresion), have you tried using redirects, i.e.

propka > std.out

which will remove the output you do not need. Alternatively, if you feel like discarding it entirely you can always redirect to /dev/null instead. This will not impact your own scripts at all.

Returning to the the proposed set of changes you have given in this pull request I think that the output suppresion should, for now, be discarded or at least put in issue #10. Your bug-fix regarding the input parameter processing is of course a welcome addition.

Perhaps @charnley can comment on what he thinks and how to proceed.

@dotsdl
Copy link
Author

dotsdl commented Jan 27, 2015

I think there may be a bit of confusion on the motivation for these changes. I am not running propka from the shell, but instead feeding propka.run.single() a PDB structure via a named pipe. I then access the results from the Molecular_container object to get what I need. Any files written to disk are not used, and this is all done within python, not a shell.

The output displayed to stdout is a separate issue, and one that I haven't addressed at all in these commits. I realize that adding options for suppressing printed output isn't a particularly exciting feature, but it makes it much easier to envision propka being used on whole trajectories inside of larger codebases if this were the case.

@cstein
Copy link
Member

cstein commented Jan 28, 2015

Ahh, I understand now and can see why your particular setup would require this suppression.

The propka code is littered with print(str) statements, so you are indeed correct that it is not the most exciting thing to implement. Let's keep the discussion on the output suppression in #10.

@orbeckst
Copy link
Collaborator

@dotsdl I think you can close this PR. The --quiet keyword does what was needed.

@dotsdl
Copy link
Author

dotsdl commented Apr 14, 2017

@orbeckst yeah probably fine to close this for now. Will check out the changes from #12.

@dotsdl dotsdl closed this Apr 14, 2017
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

3 participants