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

Adding new features: demand weight and distance matrix #93

Open
huanfachen opened this issue Nov 27, 2020 · 2 comments
Open

Adding new features: demand weight and distance matrix #93

huanfachen opened this issue Nov 27, 2020 · 2 comments

Comments

@huanfachen
Copy link

Hi Nick. I plan to add two new features to the max_coverage function:

  • Adding demand weights
  • Accepting a distance matrix

These features would make this package more general, which are motivated by my recent work on comparing open-source packages for location cover models. More details are here.

I would like to push these new features to this repo and would like to know your thoughts on this. Many thanks!

Best Regards,

Huanfa

@mpadge
Copy link
Contributor

mpadge commented Nov 28, 2020

@huanfachen Much of this has already been documented in previous issues. Inclusion of distance matrices is in #77, including all code which will eventually be merged, and the weights issue is #85, which Nick and I have discussed a lot, although not much currently written in that issue. You'll also notice that both of those issues have been lying around for quite some time. Nick and I both (obviously) have other priorities preventing them from being addressed, so maybe among the most useful things for you to do would be:

  1. Pressure us to get these features incorporated and working; and even more importantly
  2. Make sure you watch this repo and help contribute to/test/modify/comment on any code that gets PR'ed in relation to the relevant issues (Add support for OSM dodgr distances #77, Add weights as an option for maxcovr #85).

Sound good? I guess you're in relatively frequent contact with Nick, and he knows best ways to nudge me to get us all working together on finishing these missing pieces. And finally: Great paper, and a really important contribution to open-source advocacy! Thanks for writing it, and linking here.

@huanfachen
Copy link
Author

huanfachen commented Dec 1, 2020

@mpadge Thanks for your comments. I have implemented a max coverage function called max_coverage_weighted that incorporates demand weight. It still needs documentation and more testing, but it is working for my cases. Hope this is something useful.

In addition, I have some suggestions on the max_coverage function:

  1. Not all applications contain a non-empty existing facility set. Therefore, this function should allow existing_facility as NA (or NULL) and skip the step of testing which users have been covered by existing facilities.
  2. The order of proposed facilities in the argument of d_proposed_facility and demand_weight (an object that contains demand weight) should be the same. This needs to be explicitly stated and/or tested in the function.

Thanks for working on this package, this is so helpful.

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

No branches or pull requests

2 participants