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

deleted commented lines #159

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

deleted commented lines #159

wants to merge 1 commit into from

Conversation

robfairh
Copy link
Contributor

Fixes #158

@robfairh robfairh added Comp:Core This issue has to do with the main bulk of the code or document. (methods, main content) Difficulty:1-Beginner This issue does not require expert knowledge and may be a good issue for new contributors. Priority:2-Normal This work is important and should be completed ASAP. Status:5-In Review This issue has been handled, and the solution is being reviewed. (probably as a PR) Type:Style Is related to coding style guide compliance (e.g. PEP8 or google C++ style guide). labels Feb 25, 2021
Copy link
Collaborator

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

We should go through all .sql files, .py files, and the Snakefile to check for cruft before completing the analysis.
At the moment, the lines deleted in this pull request are being used for development purposes.

@@ -14,10 +14,6 @@ DBS = ["UIUC_2050_cost"]
SCENARIOS = ["costScenario"]
FOLDER = ["UIUC_2050_cost_costScenario_model"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# uncomment these lines to run the land minimization scenario.
# DBS = ["UIUC_2050_land"]
# SCENARIOS = ["landScenario"]
# FOLDER = ["UIUC_2050_land_landScenario_model"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a comment explaining the purpose of those lines.

At the moment, we are making changes to one scenario at a time. Not a good idea to delete them right now.

Instead, we should change the # All Scenarios block, because none of those will be a part of the final analysis.
If we must delete comments, then remove the All Scenarios block.

Copy link
Member

@katyhuff katyhuff Feb 25, 2021

Choose a reason for hiding this comment

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

Surely snakemake has a more graceful way to do this? (that is, more graceful than commenting and uncommenting depending on the moment... )

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that is the way to do it, based on the way I've written the rules.
You can limit the rules that are executed with the --until flag, but the current rules will run every scenario listed in the DB, SCENARIO, and FOLDER variables.

The only alternative I can think of is to write one rule for each scenario, but then the snakefile is repeating code. This was the approach I took in the snakefile for cairo, but each scenario actually has a different command line argument and snakemake doesn't like it when the wildcards aren't the same.

Copy link
Member

@katyhuff katyhuff left a comment

Choose a reason for hiding this comment

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

This doesn't fix #158, as the PR description claims. #158 is not just about one block of commented lines. This one block was an example. Issue #158 says:

This issue can be closed when someone has gone through the repository and removed this and all other commented-out lines that are not in use.

@robfairh
Copy link
Contributor Author

robfairh commented Feb 25, 2021

This doesn't fix #158, as the PR description claims. #158 is not just about one block of commented lines. This one block was an example. Issue #158 says:

This issue can be closed when someone has gone through the repository and removed this and all other commented-out lines that are not in use.

Oh, my bad. I went through it too quickly. Then, I will leave this PR as a draft. There is a ton of things commented out in the models that we should go over in order to fix #158.

@robfairh robfairh marked this pull request as draft February 25, 2021 17:00
@yardasol yardasol removed their request for review January 18, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Comp:Core This issue has to do with the main bulk of the code or document. (methods, main content) Difficulty:1-Beginner This issue does not require expert knowledge and may be a good issue for new contributors. Priority:2-Normal This work is important and should be completed ASAP. Status:5-In Review This issue has been handled, and the solution is being reviewed. (probably as a PR) Type:Style Is related to coding style guide compliance (e.g. PEP8 or google C++ style guide).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean-Up Commented lines
3 participants