Skip to content

RunDeepProfiler#182

Open
fefossa wants to merge 13 commits into
CellProfiler:masterfrom
fefossa:run-deepprofiler
Open

RunDeepProfiler#182
fefossa wants to merge 13 commits into
CellProfiler:masterfrom
fefossa:run-deepprofiler

Conversation

@fefossa

@fefossa fefossa commented May 3, 2023

Copy link
Copy Markdown
Contributor

No description provided.

@callum-jpg

Copy link
Copy Markdown
Contributor
  • Work out a strategy for installing CellProfiler and DeepProfiler in the same python environment
  • Work out how to run DeepProfiler repeatedly with the same experiment_name but changing image input

@fefossa

fefossa commented May 24, 2023

Copy link
Copy Markdown
Contributor Author

Work out how to run DeepProfiler repeatedly with the same experiment_name but changing image input

  • So DeepProfiler needs an index.csv file that contains the plate, well, site, and filename for each channel. I created 2 index.csv files with different images as the input. Then, I called python3 deepprofiler --root=/home/ubuntu/project/ --config filename.json --metadata index.csv --exp experiment_name profile and only changed the --metadata filename.
  • DeepProfiler runs fine with the same experiment_name but changing index.csv file.

@bethac07 bethac07 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! A couple tiny comments around print statements, hardcoding of Windows-style paths, and subprocess nuance, but otherwise, works great!

Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
Comment thread active_plugins/rundeepprofiler.py Outdated
@fefossa fefossa marked this pull request as ready for review June 2, 2023 15:13

@bethac07 bethac07 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve on the code basis, but someone else (@ErinWeisbart ?) should check if anything else is needed on our list of things we ask for in a submission now

@ErinWeisbart

Copy link
Copy Markdown
Member

Does this work with the citation generator?
Would you please add it into the supported plugins doc? https://github.com/CellProfiler/CellProfiler-plugins/blob/master/documentation/CP-plugins-documentation/supported_plugins.md

@fefossa

fefossa commented Jun 2, 2023

Copy link
Copy Markdown
Contributor Author

Does this work with the citation generator? Would you please add it into the supported plugins doc? https://github.com/CellProfiler/CellProfiler-plugins/blob/master/documentation/CP-plugins-documentation/supported_plugins.md

Yes, it works with the citation generator. Documentation added. Just added a page for RunDeepProfiler as well so we can edit instructions as we change stuff.

@ErinWeisbart

Copy link
Copy Markdown
Member

@fefossa The install instructions you added don't match the strategy we describe in https://github.com/CellProfiler/CellProfiler-plugins/blob/master/documentation/CP-plugins-documentation/using_plugins.md

Does it not work to use the setup.py-based strategy that Callum has set up? If not, we need to add a description to the using_plugins page telling people to follow alternative instructions for RunDeepProfiler. If it does, can you add the dependencies to setup.py and then I think the whole first section of RunDeepProfiler.md doesn't need to exist as it is a duplication of using_plugins?

@fefossa

fefossa commented Jun 2, 2023

Copy link
Copy Markdown
Contributor Author

@fefossa The install instructions you added don't match the strategy we describe in https://github.com/CellProfiler/CellProfiler-plugins/blob/master/documentation/CP-plugins-documentation/using_plugins.md

Does it not work to use the setup.py-based strategy that Callum has set up? If not, we need to add a description to the using_plugins page telling people to follow alternative instructions for RunDeepProfiler. If it does, can you add the dependencies to setup.py and then I think the whole first section of RunDeepProfiler.md doesn't need to exist as it is a duplication of using_plugins?

We can use setup.py yes (I'll push that), but there's a git clone and pip install for DeepProfiler itself before doing the setup.py in CP-plugins; do we think these instructions can go inside using_plugins as a note? @ErinWeisbart

@fefossa

fefossa commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

@ErinWeisbart instructions are updated!

Option to add the measurements to the exporttospreadsheet/database and display those measurements on the window
@fefossa

fefossa commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Now I've added the option to export the results into a CellProfiler format (within the CSVs).

Because I'm on Windows I still cannot test on analysis mode if the files are being written to the spreadsheet, but I can see on the display window below that the way I'm getting the measurements looks correct.

If you could test on Mac @bethac07 thank you!

image

@bethac07

Copy link
Copy Markdown
Member

@fefossa Sorry, no, I can't get it to run on my computer unless/until we go with the Docker version, so I can't check, sorry! We'll need someone with a non-M1 mac - which might of the current team just be @gnodar01 , we can hopefully have him check later this week!

@bethac07

Copy link
Copy Markdown
Member

Actually, @ErinWeisbart, do you have a non-M1 mac?

@ErinWeisbart

Copy link
Copy Markdown
Member

I do have a non-M1 Mac. I can test this later today. Stay tuned...

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.

4 participants