-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 the phenotype module #755
Add the phenotype module #755
Conversation
Provides access to PlateRecord and WellRecord objects PM data can be read from csv or JSON files PM data can be written to JSON files
Three sigmoid functions are used This approach needs scipy, a warning is raised if it's missing
An initial guess of the curve parameters are added: without this, the sigmoid function fitting was incorrect in most of the cases.
This reverts commit 28a0c3f. The wrong file has been modified in the previous commit
Including more PEP8 changes
Current coverage is
|
@@ -390,6 +390,7 @@ def is_Numpy_installed(): | |||
'Bio.Affy', | |||
'Bio.Cluster', | |||
'Bio.KDTree', | |||
'Bio.phenotype' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally that would have included a trailing comma, so that next time an entry is added to the list it is a one line change. But there's no real need to change it now.
Your code includes a lot of Is this something you've tried? Unfortunately making doctests testable examples can be a lot more work due to things like this and cross-platform differences, and sometimes we have to make slightly less friendly examples - or use deliberately tiny example files (e.g. see the
Related to this, via Note: I don't think this potential polishing should delay merging the work so far. |
>>> subplate = plate.subtract_control() | ||
""" | ||
|
||
def __init__(self, plateid, wells=[]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't use a mutable default argument, would wells=()
work here? Otherwise use wells=None
and set this to a (new) empty list within the __init__
method.
This is one of the problems checked for by the quantifiedcode automated checks, also pylint which calls it W0102, see e.g. http://pylint-messages.wikidot.com/messages:w0102
Ok, all the suggested changes are in (except for the doctests, which could be added later I guess?) |
Hi Peter, I didn't see any comment on this PR so far; would you like me to inquiry a bit more to see if the changes are sound enough? Best, |
Hi Peter, so far I didn't see any comments regarding this proposed module. Did you see/hear anything? Best, |
I'm happy, just haven't had time outside of work to apply your changes yet. Sorry. |
Hi, importing the phenotype module will now trigger a warning. |
I had some issues rebasing this - which I think was due to using a Mac with its default case insensitive file system (like Windows) and git not coping well with the folder renaming. I ended up compressing your work into one commit with some minor changes included like removing trailing white space in the tutorial and renaming another module to be lower case ( |
Thanks, will have a look right now |
Sorry, to have a cleaner history and avoid any mistake I preferred doing a new PR.