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

Abstreet new sites scenarios #169

Merged
merged 10 commits into from
Apr 23, 2021
Merged

Conversation

natesheehan
Copy link
Contributor

Sanity checks all passing ✅

This PR contains

  • compiled scenario_base.json, and scenario_go_active.json for the new sites added last week
  • new site "newborough-road"
  • refactoring of build.r to reintegrate abstr scenario autogeneration
  • refactoring of abstr-scenarios.r for new sites (abstr_scenarios.r not compatible for new sites #159)

Copy link
Contributor

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

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

Looks good to me. Are these based on the new procgen houses using the approach @dabreegster mentioned on Slack? An improvement in any case for sure, great to see the buildings in there, those can go straight up on the web app when we merge this.

}
# Populate site metrics for new site --------------------------------------

if(new_site){
if (new_site) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 in terms of style an improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah think this was an accident code format change. I guess if we wanted, we could come up with a script styling guideline.

@@ -27,6 +27,7 @@ lockleaze,Lockleaze,Bristol,no,150,https://www.bristolpost.co.uk/news/bristol-ne
long-marston,Long Marston Garden Village,Stratford-upon-Avon,no,3500,NA,7,4,6,10,11.8,9.8,1,5,1,82,3,2,7,1.59,NA,3.11,NA,NA,NA,79,0,10,red,red,red
marsh-barton,Marsh Barton (Livable Exeter),Mid Devon,no,5544,https://www.liveableexeter.co.uk/garden-communities/garden-communities/marsh-barton/,30,51,61,58,1.9,2,3,22,8,53,9,1,7,1.45,1.26,3.98,2.33,2.37,4.07,25,32,26,NA,NA,NA
micklefield,Micklefield,Leeds,partly,292,https://publicaccess.leeds.gov.uk/online-applications/applicationDetails.do?keyVal=NMH4Q7JB17S00&activeTab=summary,5,6,22,14,14.6,6.4,8,4,1,75,8,6,7,1.22,NA,4.13,2.02,2.03,23.28,66,0,14,red,red,red
newborough-road,Newborough Road,Peterborough,no,1130,https://www.peterboroughtoday.co.uk/lifestyle/homes-and-gardens/plans-1000-new-homes-peterborough-farmland-be-submitted-3029332,8,23,27,27,4.7,6.9,2,3,5,76,6,3,8,1.78,1.98,2.47,NA,NA,NA,57,3,24,NA,NA,NA
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to see another site in there.

@natesheehan
Copy link
Contributor Author

natesheehan commented Apr 23, 2021

Looks good to me. Are these based on the new procgen houses using the approach @dabreegster mentioned on Slack? An improvement in any case for sure, great to see the buildings in there, those can go straight up on the web app when we merge this.

Nope, not based on the new approach yet, this PR is mostly housekeeping . Should/do we import new sites to abstreet before we start using the new procgen house tool @dabreegster @Robinlovelace ?

@dabreegster
Copy link
Collaborator

Should/do we import new sites to abstreet before we start using the new procgen house tool

No, the other way around. I don't think you can generate the base/go active scenario JSONs without the procgen houses for some sites. So I would recommend

  1. Adding in the procgen houses
  2. Generating the scenario JSON
  3. Doing the abstreet import once everything in data-small isn't going to change. (And recall that ultimately I have to run those steps, because of uploading to S3)

@Robinlovelace
Copy link
Contributor

Any harm in merging this now @dabreegster ? I think not and it will be easy to update the relevant files after running the procgen import process but wanted to check in with your first from an abst perspective 👀

@dabreegster
Copy link
Collaborator

Any harm in merging this now

Nope, go for it

@Robinlovelace Robinlovelace merged commit b8cca0a into main Apr 23, 2021
@Robinlovelace Robinlovelace deleted the abstreet_new_sites_scenarios branch April 23, 2021 15:59
@natesheehan
Copy link
Contributor Author

newsite

front end looks all good for actdev

@Robinlovelace
Copy link
Contributor

🎉 fantastic. Many thanks for the great PR and checking it all works.

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