StatistikStadtZuerich / BevSzen

population scenarios City of Zurich
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Request/bzo2040 #63

Closed insilentio closed 1 year ago

insilentio commented 1 year ago

code for customer request 5839-01

insilentio commented 1 year ago

My comments to reviewer's input in italic:

Overall

  • no errors found; only minor suggestions (see below)
  • an overall workflow (first KaReB_Datenaufbereitung.egp, second BEV570.egp etc) and/or a documentation were helpful for process traceability
  • in reviews it were helpful to see (at the very beginning of the review) the plots as well; I prefer to keep the plots

not sure what you mean: add plots to github?

KaReB_Datenaufbereitung.egp

  • KaReB_Datenaufbereitung.egp: SAS-Code is ok; however, we should avoid manual steps in future (e.g. copy and manipulate Excel files)
  • Only keep needed process flows (probably process flow 3 only)

BEV570.egp

  • name: BEV570.egp; better not to use a SASA name; different content = different name
  • error messages in the OGD to HDB program; why? who made this code? Ugly coding style

for the SAS part probably best if you could show me live what you mean

1504_output_ssd.r file renaming is peculiar (but idea is ok; kareb2040 as base areas; kareb2016 as reference); unconvenient: the code cannot be run multiple times (since the files are renamed)

added an if-statement to be able to run it

  • line 40: pop_middle does not exist? first run the entire code for BZO2016? Code should run without additional files

added comment

  • after running lines 44 to 46 (run_scen), I had no object pop_middle; what went wrong? Consequently, I can only run the population comparison plots with dummy data

_added sourcing of 1501_modeloutput.r

  • line 51: better case_when (for 3 or more classes)

changed

  • line 62 (and before): why another folder 'requests'? We already have a folder for outputs

I think it is a good idea to keep default outputs and customer request based outputs separate

  • line 70: why not bind_rows?

don't know, I like union :-)

  • line 114: I can not open plot 15A2_car_dcat; what went wrong?

probably some mix-up with the plot fix branch? I included the changed plot function into this branch and re-ran the plots; they work here

  • line 144: same here (plot 15A4_bzo40_last_2_years_per district)

see above

  • line 199: not possible to run (since I use GitHub on my own laptop)

I can send you the file if needed

Results

  • I corrected minor typos (e.g. year)
  • BevSzen as additional source

directly in the Excel or where?

insilentio commented 1 year ago

reviewer confirmed re-approval via phone