-
Notifications
You must be signed in to change notification settings - Fork 6
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
Run plates 3 and 3prime #10
Run plates 3 and 3prime #10
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
LGTM after some comments are addressed!
@@ -77,6 +80,38 @@ | |||
output_dir=output_dir, | |||
metadata_dir=metadata_dir, | |||
figshare_url=figshare_url, | |||
unzip_files=unzip_files, | |||
unzip_download="True", |
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.
consider adding an if statement for if the files are zipped or not to make this not break and reusable
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.
@MikeLippincott Can you explain what you mean a bit more here? Are you saying in the download_figshare
function to create an if
state to stop the function if the individual puts unzip download when the files being extracted are not zipped?
@MikeLippincott, When you have a chance, can you please re-review this PR? Thank you! |
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.
Nice work! I generally thought things LGTM except for a block covering pd.read_sql()
(this is I feel the primary reason for my requesting changes). Please don't hesitate to let me know if you have any questions or if I may clarify on any comments.
@@ -12,7 +12,9 @@ To calculate and apply an IC function on each channel, run the [nf1_ic.ipynb](nf | |||
# move to the 1.cellprofiler_ic directory to access the `sh` script | |||
cd 1.cellprofiler_ic | |||
# run the notebook as a python script | |||
bash nf1_ic.sh | |||
source nf1_ic.sh |
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.
There could be issues but I don't feel concerned in this instance. There are a lot of different ways one can configure their terminal environment; using source
into the current shell could mean one runs into personal terminal / development issues (environment variables, settings, other system configurations, etc).
To get around permission issues specifically, you could issue chmod +x <filename>
, which may allow you to execute the file without specifying the shell type or sourcing it into the current shell. This seems like it might allow for the script to run on MacOS (zsh) and other bash-based OS's terminals. It's also possible to install and use bash
through brew
on Macs.
Generally, if executing shell script as part of the project, I'd wonder what environment the shell should execute in (what OS, which shell, etc). This gets into more challenging spaces for reproducibility and I only bring this up to provoke thoughts.
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.
Great work on the changes - LGTM!
|
||
image_query = f"select * from {image_table_name}" | ||
image_df = pd.read_sql(sql=image_query, con=conn) | ||
image_df = pd.read_sql_table(image_table_name, f'sqlite:///{sqlite_file_path}') |
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.
🙌
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.
Nice!
@d33bs Thank you for that quick rereview! I will now merge these changes! |
In this PR, we:
All intermediate file from processing CellProfiler features have been uploaded to GitHub.