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

Save interpretations #12

Merged
merged 12 commits into from
Feb 13, 2023
Merged

Save interpretations #12

merged 12 commits into from
Feb 13, 2023

Conversation

roshankern
Copy link
Member

This PR is ready for review!

In this PR, model coefficients are saved for the final and shuffled baseline models. These coefficients are saved in the tidy long standardized data format.

roshankern and others added 11 commits December 9, 2022 15:51
* finish download module changes

* download notebook

* rerun split data module

* rerun download module

* rerun train_model

* rerun evaluation module

* rerun interpretation module

* combine datasets

* combine datasets

* split changes

* update format

* format update

* format

* finish split data

* combine datasets, remove holdout

* formatting

* rerun pipelines

* remove folded class

* rerun pipeline

* Update utils/download_utils.py

Co-authored-by: Dave Bunten <[email protected]>

* PR fixes

* module docstrings

Co-authored-by: Dave Bunten <[email protected]>
@roshankern roshankern requested a review from d33bs February 13, 2023 17:47
Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

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

LGTM

A couple discussion items that you may decide to change:

  • I think adding the term coefficients to the notebook (currently interpret_model.ipynb) might be more explicit about what going on
  • Currently, the features are being saved as their index? Is this true? Could you instead use their feature name? This will prevent the scenario of incorrect coefficient index assignment.
  • Are these features DP or CP? It's probably worth specifying in the file name.

Copy link
Member

@d33bs d33bs left a comment

Choose a reason for hiding this comment

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

Nice work! I only had minor comments and suggestions with this review for your consideration. Overall LGTM!

4.interpret_model/scripts/nbconverted/interpret_model.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
4.interpret_model/scripts/nbconverted/interpret_model.py Outdated Show resolved Hide resolved
@roshankern
Copy link
Member Author

In response to @gwaybio,

Suggestions 1 and 2 have been implemented in bc1bf11.
Regarding suggestion 3, I don't think its worth specifying that these features are from DeepProfiler, as so far everything in the repository is DP. Once the final changes are done for this repo, I will make a repo version for multi-class model - DP and the next version will be something like single-class models CP and DP.

@roshankern roshankern merged commit df2bfb6 into WayScience:main Feb 13, 2023
@roshankern roshankern deleted the save-interpretations branch February 13, 2023 21:52
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.

3 participants