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

Catherine Harvey Project Outline #4

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

Conversation

harveycas
Copy link

No description provided.

@leej3
Copy link

leej3 commented Mar 19, 2020

Looks like a nice project.

Does it have to be excel? Using non-proprietary formats like csv can be far more useful from the perspective of open science.

I strongly encourage you to fully explore the python package pandas before you do too much planning. Some of the datacamp courses teach its functionality. This will help you to get a sense of how the python community thinks about and works with this sort of tabular data.

harveycas and others added 5 commits April 23, 2020 18:38
Need to add additional features like graphs but this is the beginning
want to add graphs for my data.  In addition, I was wondering if this script is on the right path for the final project?
@harveycas
Copy link
Author

harveycas commented Apr 30, 2020

I was wondering if I could have my draft checked for the final project? I used JupyterLab to create my data and it comes out properly when I look at the file from my folder. However, when accessed here it doesn't look right

@harveycas
Copy link
Author

I added a file defining my code and I was wondering if it's ok? In addition, I am still not sure regarding how to test my code and produce tests for them. I was wondering if I could have a bit of clarification? Thank you!

@leej3
Copy link

leej3 commented May 6, 2020

This is starting to look much better.

Some pointers:

  • you have indentations of 1 and 5 spaces. They should all be multiples of 4.
  • For my example the sample file path should have instead been: SAMPLE_XLSX = Path(__file__).parent / "data" / "python_test_list.xlsx"
  • For your test of antibiotic_analysis you will need to call it and then read in the excel file and check that it matches a sample excel file. I would worry about this one though. That would be called an integration test. What I want to see is more along the line of unit tests. They are sanity checks to make sure as you change your code, it still maintains the same basic functionality.
  • A similar situation for write_marker_dict_to_disk. You could give it a test path and a small dict of data frames to check that it writes output to disk but no need to do more in depth value comparisons.
  • Focus your effort on clearly documenting what you expect sort_and_filter_by_col and combine_antibiotics to do. Given a specific input (that you can create from the sample xlsx file) you should have a clear output value you expect. Sub sampling the data like I have done in test_read_excel_file will likely be essential to make it manageable.
  • you will need to add xlrd as a dependency to pass the tests

EDIT: I meant "would not worry about this one above"

@leej3
Copy link

leej3 commented May 8, 2020

I've suggested some fixes. Looks like you already made some. No worries. Reorganizing to functions and add tests all at the same was a bit much. I should have submitted smaller changes. Apologies

Overall well done. It's nice work. You grappled with tests which is what I wanted. You are not too far off passing them. Hopefully you got a feel for how they would make things easier and how they reveal bugs (unfortunately you had a lot to work through post merge).

@emilyyaklich
Copy link

Hi Catherine,

Your project is very well done, good work! I really appreciate the thorough documentation. Your unit tests are also nice as well.I think in the future, it might be helpful to update your .gitignore file with data/directories that you don't want in your final repository (the .ipynb_checkpoints or the miscellaneous-don't read). Overall, I think your project is good and it is clear to follow. Great job!

Cheers,

Emily

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