search

npct / pct-shiny

The Shiny map for Local Authorites
GNU Affero General Public License v3.0
24 stars 14 forks source link

using codebook to select variables to show in download #446

Closed AnnaGoodman1 closed 7 years ago

AnnaGoodman1 commented 7 years ago

I see that the variables 'serror' and 'ferror' have now been removed from the fast/quiet routes geojson files, which is good, there are still a large number of other variables in there that are not defined in the codebook.

In an email on Saturday, @Robinlovelace said "I've just implemented code that ensures we only keep the variables named in th code-book for rf, rq and rnet and put it in a PR: https://github.com/npct/pct-load/pull/66". could this be integrated to the latest IOW build?

nikolai-b commented 7 years ago

Can we update the codebook? The additional variables are the ones attributes for the straight lines e.g. govtarget_sld so it gives the routes more meaning.

Robinlovelace commented 7 years ago

Hi @nikolai-b I think we agreed not to include information form PCT in the fastest and quietest routes, otherwise it's duplicating information. Also cycling uptake is at the desire line level: the route the additional cyclists take is rather subjective. But the DRY argument is the strongest for me - that's why we created the codebook to ensure only vars we want are in there. Will take a look but looks like my code to selected only the vars in the codebook has been reverted or isn't working...

Will check now but, to be clear, the variables that should be in rf and rq are here: https://github.com/npct/pct-shiny/blob/master/static/codebook_routes.csv

Robinlovelace commented 7 years ago

OK just spotted the issue: my code successfully removes excess vars from rf.Rds. However, this line of code re-adds them (now that's what the user gets due to on-the-fly downloads):

https://github.com/npct/pct-shiny/blob/8c048b96bf67c9709eb1c0c73c45b2fe6cd73e69/server-base.R#L67

AnnaGoodman1 commented 7 years ago

thanks both. I think it is very good to implement this code rule, to make sure that internal variables don't accidentally creep in. @Robinlovelace , personally I would be fine with adding the scenario variables in there like govtarget_sld, as @nikolai-b suggests, I think it would make the downloads a bit more user-friendly (since they don't need to be merged) athough I appreciate it introduces a small amount of repitition. If we did do the update, I could write the updated codebook. Or we could leave it as it is and add them later if users ask for this.