njtierney / maxcovr

Tools in R to make it easier to solve the Maximal Coverage Location Problem
https://njtierney.github.io/maxcovr/
GNU General Public License v3.0
43 stars 12 forks source link

Appropriately handle `lat` and `long` columns and add flexibility #63

Open njtierney opened 6 years ago

njtierney commented 6 years ago

At the moment maxcovr assumes that all columns with latitude and longitude information are named lat and long.

This is not ideal, as it throws a cryptic error message when these are not given the appropriate names.

There should be three steps in refactoring:

  1. Add a stop function that stops the analysis if things are not named lat and long. This can then be used in each function that uses that info.

  2. Allow the user to specify their own lat and long arguments

  3. Cleverly detect different variations on lat and long as leaflet does and display a nice message

mpadge commented 5 years ago

I've got this kind of thing in dodgr with an internal dodgr_graph_cols function, which relies on a whole bunch of helper functions here. It's kinda messy, but works really well at auto-identifying standard columns based on unknown and variable input names. The idea is to return a list so everything else then works like this:

cols <- dodgr_graph_cols (graph)
lon <- my_input [[cols$lon]]

... or whatever. Point is, it's robust and flexible: Write a function to auto-identify the columns you need; return those as list items; then use code like the above to extract when needed.

njtierney commented 5 years ago

OK that is rather neat!

Am I correct in that we are looking for something like this to identify say x and y cols:

mpadge commented 5 years ago

yeah, the details are all pretty easy. The difficult and important thing was figuring out that the get_columns function should return a list. From that point on, it's all easy