Skip to content

Validation function #27

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

Merged
merged 23 commits into from
Dec 4, 2019
Merged

Conversation

wagenrace
Copy link
Collaborator

@wagenrace wagenrace commented Nov 27, 2019

issue #26

  • Add functions to have metrics separate from experiments itself.
  • Make notebooks into scripts (this was needed to import utils from every place)
  • reduce amount of code
  • run experiment 5 times to find out if results are constant

I performed the following prior to filing this pull request:

  • Tested that my change does not break the analysis pipeline
  • Ran a linter through my code
  • Update environment dependencies if my code introduces a new package

@wagenrace wagenrace requested a review from gwaybio November 27, 2019 15:23
Copy link
Contributor

@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.

This is great - adding F1 score and confusion matrix code is very important. I made several comments on most files, but I will summarize my main points below:

  1. I highly recommend moving away from .xlsx files to .csv or .tsv files. .xlsx files are much heavier weight than what we need for this project.
  2. I couldn't tell how the model was being trained with 6 different initializations. Please clarify.
  3. We need to make sure you're properly getting credit for these important contributions! I am seeing grey boxes which indicate that the commit is not properly being attributed to your username. Make sure that the email address you're using to make the commits on your local machine is linked to your github account.

Copy link
Collaborator Author

@wagenrace wagenrace left a comment

Choose a reason for hiding this comment

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

@gwaygenomics that should be it

Copy link
Contributor

@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.

Nearly there - only a couple minor and optional comments and just some additional cleanup required.

  • There are still some confusion_matrix.xlsx files here. Lets replace them with .csv files.
  • Please remove the binary file ~$all_scores.xlsx

Otherwise, looks great! 🎉

@gwaybio
Copy link
Contributor

gwaybio commented Dec 4, 2019

@wagenrace - confirming that you are not waiting on me - just a couple .xlsx files to be replaced with .csv and the one ~$all_scores.xlsx file to be removed.

Then we are good to merge! 🎉

@wagenrace wagenrace merged commit db96e3b into cytodata:master Dec 4, 2019
@wagenrace wagenrace deleted the validation_function branch December 4, 2019 12:59
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.

2 participants