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

Final project: Morgan Kindel and Julia Burke #6

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

Conversation

morgankindel
Copy link

No description provided.

jnburke4 and others added 2 commits February 27, 2020 19:53
Julia and Morgan's project outline
@leej3
Copy link

leej3 commented Mar 19, 2020

Looks good. I strongly encourage you to use pre-existing tools to achieve all of these details. Your project will be creating a pipeline to run all of those tools. Snakemake or Prefect would be two tools that will help you create such a workflow.

See my comment here about working together.

See my comment here about balancing the project goals of ticking off details on the rubric etc.

@leej3
Copy link

leej3 commented Mar 19, 2020

@jnburke4

morgankindel and others added 13 commits May 7, 2020 22:32
1. Import files
load input image from Sample images file folder or set new path for new input image
Create the reference image set up, includes set up to iterate through several images as well as grid for matching template.
creates a copy of template grid to put the box on, indicating the most similar image
Merge edits_BurkeKindelproject
@leej3
Copy link

leej3 commented May 8, 2020

Can you guys just leave a comment here about how the merging/merge conflicts went. I'd like to get a sense of how much of time you lost to that.

Also your code is nicely modularized in that it is split into notebooks. If you can make functions out off the some of the code there some easy points there. And some docstrings and assertions (or even tests).

@morgankindel morgankindel reopened this May 8, 2020
@morgankindel morgankindel changed the title Update README.md Final project: Morgan Kindel and Julia Burke May 8, 2020
@jnburke4
Copy link

jnburke4 commented May 8, 2020

Can you guys just leave a comment here about how the merging/merge conflicts went. I'd like to get a sense of how much of time you lost to that.

Also your code is nicely modularized in that it is split into notebooks. If you can make functions out off the some of the code there some easy points there. And some docstrings and assertions (or even tests).

The merging was surprisingly extremely difficult, I couldn't say how much time we have spent sorting out where to put things but it was many hours. Ultimately, we decided since most of our work was here in this repository, we would post here. We merged some but this was working out better.
We split functions into tests as well, thanks for the suggestions!

@leej3
Copy link

leej3 commented May 8, 2020

Ok good to know thanks. Yea we didn't work through that. There are various strategies to make it easier. It is especially difficult with Jupyter notebooks though.

@leej3 leej3 closed this May 8, 2020
@leej3 leej3 reopened this May 9, 2020
@leej3 leej3 closed this May 9, 2020
@leej3 leej3 reopened this May 9, 2020
@leej3
Copy link

leej3 commented May 9, 2020

Excellent job. I appreciate the toils with trying to resolve git merge conflicts. I didn't know, otherwise I could have provided some strategies for improving the situation. With that said you still created a nicely organized, modular project, with lots of comments.

Areas for improvement for your future in coding:

References to pre-existing solutions could be better. You use opencv to solve your problem: mentioning this and the reasons for this decision would be useful for others considering reusing your code. Even if "my lab uses it" that's fine but demonstration of awareness of the context to your decision is important

Look at some of the other students' projects regarding testing: adding tests in the tests directory will make any projects far more scalable and maintainable. Especially in a collaborative setting, tests provide sanity to the development process, ensuring that changes introduced do not break previously working functionality.

Python modules should typically define functions and not run them or much other code (besides imports and some general variable declaration).

If you do wish to run the file as an executable too you can wrap that bit of code like this:

__name__ == '__main__':
    filecount()
    create_dict()
    create_template()
    match_images()

You have stored a few too many images in the repository. Ideally you would store this somewhere else. It quickly become unmanageable to work with git if you have such a large size on disk. Solutions include:

  • Make the data much smaller (often impractical)
  • storing the images separately on a webserver
  • storing the data in a git submodule
  • using git-annex or datalad

Regarding the "package":
In your setup.py file you are missing a comma after the url argument and you should include your dependencies in the install_requires argument (opencv-python, PIL etc)

Overall your attention to python style is good. A notable exception is lines of code that are too long. Moving forward keep an eye on the pep8 style guide to continue to grow in this respect

Well done.

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