8000 Partial solution to #18 by JamesSaxon · Pull Request #26 · pysal/access · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Partial solution to #18 #26

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 6, 2022
Merged

Partial solution to #18 #26

merged 5 commits into from
May 6, 2022

Conversation

JamesSaxon
Copy link
Contributor

This solves the issue noted by @knaaptime in #18, in his notebook, that the two-stage numbers were not affected by changes in the OD matrix. (Thanks!)

This fixes a bug where it was not the weighted values that were getting summed in the two-stage calculation! This was basically a one-line bug, that the suffix "r" had been added, but then not used.

I wrote a completely "base python" gravity-based 2SFCA below, that we could add as an additional unit test.' With that fix, the package and the function below (for the four simple cases!!) agree, point-identical.

https://gist.github.com/4a3c3d1c571523db43785e43b256ba78

See if this looks reasonable?

def simple_2sfca(OD, supply = docs, demand = pops, locs = locs, max_travel = 61,
                 return_debug = False):
    
    D = {hosp : sum(demand[res] / OD[res][hosp]
                    for res in locs 
                    if OD[res][hosp] < max_travel)
         for hosp in locs}

    R = {l : supply[l] / D[l] for l in locs}
    
    A = {res : sum(R[hosp] / OD[res][hosp] 
                   for hosp in locs
                    if OD[res][hosp] < max_travel)
         for res in locs}
    
    if return_debug:
        return A, R, D
    
    return A

@JamesSaxon JamesSaxon mentioned this pull request Feb 4, 2022
Closed
@knaaptime
Copy link
Member
knaaptime commented Feb 4, 2022 •
edited
Loading

nice. I dont know that I ever would've caught that.

2 quick things:

  • Its not critical, but are you cool with making the threshold <=? I think its more commomn for people to think of the max cost as something they're willing to spend up-to but not over.
  • do you want to add someone to co-maintain? I'm happy to do it unless you'd like to propose someone better suited (I was thinking possibly @jkoschinsky, but didnt want to volunteer her for work :))

@JamesSaxon
Copy link
Contributor Author

Great, can clean up once I have time to dig into the failing tests.

Yes, totally fine with switching to <=, though I think that's logically another issue.

@jkoschinsky is great if she's willing!!

@jkoschinsky
Copy link
jkoschinsky commented Feb 5, 2022 via email

@codecov
Copy link
codecov bot commented Apr 3, 2022 •
edited
Loading

Codecov Report

Merging #26 (77f9718) into master (bfae8ed) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage   98.21%   98.33%   +0.12%     
==========================================
  Files          17       18       +1     
  Lines        1118     1204      +86     
==========================================
+ Hits         1098     1184      +86     
  Misses         20       20
Impacted Files Coverage Δ
access/fca.py 100.00% <100.00%> (ø)
access/tests/test_euclidean.py 100.00% <100.00%> (ø)
access/tests/test_floating_catchment_area.py 100.00% <100.00%> (ø)
access/tests/test_hospital_example.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfae8ed...77f9718. Read the comment docs.

@JamesSaxon
Copy link
Contributor Author

Hi @knaaptime or @jGaboardi, please have a look if you like, and merge / close.

I've fixed the failing tests (it was a unit test failure¡), and also added a new class of tests, that assert:

  • equality with a super-simple (20-line) implementation of the 2SFCA and 3SFCA
  • that Eli's expectations of the relative access levels of different travel cost configurations.

Sorry it took me so long to get to this.

@JamesSaxon
Copy link
Contributor Author

Also tagging @jkoschinsky for info.

@JamesSaxon JamesSaxon mentioned this pull request Apr 3, 2022
Open
@JamesSaxon JamesSaxon requested a review from knaaptime April 3, 2022 21:21
@JamesSaxon JamesSaxon requested a review from jGaboardi April 16, 2022 18:01
@jGaboardi
Copy link
Member

@knaaptime Can you give this PR a review?

@JamesSaxon JamesSaxon merged commit 85e9b16 into master May 6, 2022
@JamesSaxon JamesSaxon deleted the two_stage_fix branch May 6, 2022 14:39
@JamesSaxon
Copy link
Contributor Author

Hey @jGaboardi, waiting on @knaaptime, I just approved.

Anything else needed, for this to rebuild docs / version on the new site?

Or is that now all automagic? Thanks again for all your support with this project.

@jkoschinsky for info.

@jGaboardi
Copy link
Member

@JamesSaxon sorry for the super late attention here. So we need a fresh release of access?

@JamesSaxon
Copy link
Contributor Author

Hi @jGaboardi, yeah -- the PR is in, so public version should be updated.

@jGaboardi
Copy link
Member

@JamesSaxon Are you thinking patch release (v1.1.4) or a feature release (v1.2.0)?

@JamesSaxon
Copy link
Contributor Author

Fair question -- patch release, in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jGaboardi jGaboardi jGaboardi approved these changes

@knaaptime knaaptime Awaiting requested review from knaaptime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JamesSaxon @knaaptime @jkoschinsky @jGaboardi
0