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

Eric Gottlieb ndvi automation code review #29

Closed
wants to merge 8 commits into from

Conversation

esgeo
Copy link

@esgeo esgeo commented Feb 22, 2022

@eculler

Pull Request Template

Review Checklist

CI Checks

  • The notebook runs from start to finish on all operating systems:
    • Mac
    • Windows
    • Linux

Reproducibility

  • Are the data downloaded in the code
  • Are paths created to ensure they work on all operating systems (using os.path.join)
  • Are comments used to clarify the contents of the code that can’t be clarified using expressive variable and function names alone? (not too many comments - just enough)
  • Does the notebook run from start to finish?

PEP 8 standards & Code Readability

Functions

  • Do functions follow PEP 8 format conventions?
  • Are function docstrings clear (all inputs and outputs clearly described and defined)
  • Are function names expressive (the name describes what the function does)?
  • Are functions easy to understand and read?
  • How many tasks does each function do? (ideally a function does one thing well).

Package imports

  • Are standard modules (those included with the base python install) vs. third party (related but externally developed tools) import groups correct with appropriate spacing in between each group?
  • Are variable names throughout the code, expressive?
  • Suggest changes if not, highlight what was done well
  • Is the code overall easy to understand and read? Are there things that would make it more clear / cleaner?

DRY Code

  • Are segments of code repeated in the file or is the code DRY?
  • Are loops used to optimize DRY code?
  • Are functions used to optimize DRY code?
  • Are there any areas that could be potentially improved (you can suggest improvements OR you can just highlight parts of the code where you suspect it could be cleaner / more efficient.

Novel Approaches to Problem solving

  • Highlight any novel approaches to completing the assignment.

@LManak LManak mentioned this pull request Mar 3, 2022
3 tasks
@esgeo here are some notes and my review checklist for your pull request earthlab-education#29 !
Copy link
Author

@esgeo esgeo left a comment

Choose a reason for hiding this comment

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

Made changes to docstring length and code length as suggested and have pushed revised code to my branch and made PR

gottlieb-eric-ea-2022-04-ndvi-automation.py Outdated Show resolved Hide resolved
gottlieb-eric-ea-2022-04-ndvi-automation.py Show resolved Hide resolved
gottlieb-eric-ea-2022-04-ndvi-automation.py Show resolved Hide resolved
gottlieb-eric-ea-2022-04-ndvi-automation.py Outdated Show resolved Hide resolved
gottlieb-eric-ea-2022-04-ndvi-automation.py Show resolved Hide resolved
gottlieb-eric-ea-2022-04-ndvi-automation.py Show resolved Hide resolved
gottlieb-eric-ea-2022-04-ndvi-automation.py Outdated Show resolved Hide resolved
@eculler eculler mentioned this pull request Mar 4, 2022
Copy link
Author

@esgeo esgeo left a comment

Choose a reason for hiding this comment

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

@eculler I have updated my code for this assignment and committed changes to my forked repository. I am unsure whether my PR from my fork to the earthlab repository needs to be approved by you or someone else, or if it is good to go as is. Thank you for your review and guidance! Eric

@eculler eculler closed this May 19, 2022
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