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

RunDeepProfiler #182

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

fefossa
Copy link
Contributor

@fefossa fefossa commented May 3, 2023

No description provided.

@callum-jpg
Copy link
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
Copy link
Contributor Author

fefossa commented May 24, 2023

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.

Copy link
Member

@bethac07 bethac07 left a comment

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!

active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
active_plugins/rundeepprofiler.py Outdated Show resolved Hide resolved
@fefossa fefossa marked this pull request as ready for review June 2, 2023 15:13
Copy link
Member

@bethac07 bethac07 left a comment

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
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
Copy link
Contributor Author

fefossa commented Jun 2, 2023

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
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
Copy link
Contributor Author

fefossa commented Jun 2, 2023

@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
Copy link
Contributor Author

fefossa commented Jun 6, 2023

@ErinWeisbart instructions are updated!

Option to add the measurements to the exporttospreadsheet/database and display those measurements on the window
@fefossa
Copy link
Contributor Author

fefossa commented Jun 12, 2023

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
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
Member

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

@ErinWeisbart
Copy link
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