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

Retrieve data for the residential sector #6

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

samgdotson
Copy link
Member

@samgdotson samgdotson commented Aug 22, 2024

This PR includes scripts and "rules" for a snakemake workflow that retrieves residential data for the Armourdale community in Kansas City. The workflow is depicted in the file dag.png (which is generated automatically by executing the workflow).

This PR closes #5 and closes #17.

This is an unfortunately large pull request. Future pull requests should be smaller and more atomic in nature.

Why should we use pull requests to collaborate?

  1. Pull requests serve as a built-in QA/QC step when developing research code and software.
  2. The discussions within issues and pull requests document the rationale for certain decisions for future use.
  3. Critically reviewing other people's code (and having your own code critically reviewed) is a great way to improve coding skills.
  4. Pull requests don't have to be reviewed by someone working on the same project. Thus, PRs allow greater cross-program collaboration and break down silos.
  5. Similarly, PRs allow interested third parties to meaningfully contribute to open-source projects. In the case of UCS, that may include enthusiastic members of the science network.
  6. PRs protect your data and repository by preventing force pushes. In many cases, at least one other person must review the code before it is "merged" with the main code base.

Where to focus attention

The following files should be prioritized for review:

  • All files in the scripts/ folder (these should be python source files, 8 in total).
  • All files in the utils/ folder (these should be python source files, 2 in total).
  • The Snakefile file
  • The config.yml file

The jupyter notebook files can be ignored since they are experimental and will be removed in a future pull request.

How to review a pull request

This post goes over the steps for reviewing a pull request.

Recommended checklist for reviewing a pull request

The guide below is modified from the Advanced Reactors and Fuel Cycles (ARFC) research group at the University of Illinois Urbana-Champaign

  • Read the PR description
  • Read through all the changes, considering the following questions.
    • Is there anything you can praise about this PR? Start with
      praise.
    • Are variable names brief but descriptive?
    • Are new/changed functions no longer than a paragraph?
    • Do all function parameters have default values where appropriate?
    • Is the code clear and clean? (see Robert C. Martin's
      Clean Code
      • If something is unclear, ask for clarification
      • Can you offer a suggestion to make the functionality clearer?
    • Is there enough documentation?
    • Does the programming style meet the requirements of the
      repository (PEP8 for python,
      google C++ style guide, etc.)
    • If a new feature has been added, or a bug fixed, has a test been
      added to confirm good behavior?
    • Does the test actually test the new/changed functionality?
    • Does the test successfully test edge cases?
    • Does the test successfully test corner cases?
    • If the repository has continuous integration: does the PR pass
      the tests?
    • If there is no continuous integration, check out the branch
      locally, build, and run the tests.
    • Do the tests pass on your machine?
    • Is the PR free of random cruft (built files, .swp files,
      etc.)?
    • Do all files in the PR belong in the repository?
    • If the PR deletes files, is this appropriate?
    • If the PR adds files or data, are these new files compatible
      with the repository license?
    • Make a review, leaving kind comments and suggesting changes
      where needed (to resolve the above).
    • Has the author resolved all of the comments and suggestions
      in your review?
  • When you approve of the PR, merge and close it.
  • Does this PR close an issue? If so, be sure to descriptively
    close this issue when the PR is merged
  • Thank the author for their contribution.

@samgdotson samgdotson added the enhancement New feature or request label Aug 22, 2024
@samgdotson samgdotson added this to the First model runs completed milestone Aug 22, 2024
@samgdotson samgdotson requested a review from ssattler August 22, 2024 18:15
@samgdotson samgdotson self-assigned this Aug 22, 2024
@pep8speaks
Copy link

pep8speaks commented Aug 22, 2024

Hello @samgdotson! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 78:80: E501 line too long (83 > 79 characters)
Line 80:80: E501 line too long (82 > 79 characters)

Line 10:80: E501 line too long (94 > 79 characters)

Line 4:80: E501 line too long (98 > 79 characters)

Line 46:80: E501 line too long (119 > 79 characters)

Line 118:80: E501 line too long (93 > 79 characters)
Line 161:80: E501 line too long (82 > 79 characters)

Comment last updated at 2024-09-19 19:47:43 UTC

Copy link
Member Author

@samgdotson samgdotson left a comment

Choose a reason for hiding this comment

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

Test review

config.yml Show resolved Hide resolved
config.yml Show resolved Hide resolved
scripts/retrieve_census_data.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add instructions to run the workflow Add rules to retrieve data for the residential sector
2 participants