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

Networkrb #627

Merged
merged 4 commits into from
Jun 19, 2015
Merged

Networkrb #627

merged 4 commits into from
Jun 19, 2015

Conversation

sjsrey
Copy link
Member

@sjsrey sjsrey commented Jun 14, 2015

This cherry picks from #624.

However, while the unit tests pass, the doctests in https://github.com/pysal/pysal/blob/master/pysal/network/network.py will fail and need to be fixed. They evidently were not tested before the merge?

See https://github.com/pysal/pysal/blob/master/pysal/network/network.py#L253
counts isn't defined in the scope of that doctest. there are other failures as well.

We shouldn't merge this until

@dfolch
Copy link
Member

dfolch commented Jun 14, 2015

Thanks for cherry picking... these are the only two commits I had intended for #624.

@jlaura
Copy link
Member

jlaura commented Jun 14, 2015

Working on doctests now.

On Sun, Jun 14, 2015 at 3:33 PM, David C. Folch notifications@github.com
wrote:

Thanks for cherry picking... these are the only two commits I had intended
for #624 #624.


Reply to this email directly or view it on GitHub
#627 (comment).

@jlaura
Copy link
Member

jlaura commented Jun 15, 2015

Doc tests are fixed. I pulled this PR locally to make the changes using:

git checkout -b sjsrey-networkrb master

git pull https://github.com/sjsrey/pysal.git networkrb

Any thoughts on how to update this PR from my side?

On Sun, Jun 14, 2015 at 3:46 PM, Jay L. jlaura@asu.edu wrote:

Working on doctests now.

On Sun, Jun 14, 2015 at 3:33 PM, David C. Folch notifications@github.com
wrote:

Thanks for cherry picking... these are the only two commits I had
intended for #624 #624.


Reply to this email directly or view it on GitHub
#627 (comment).

@dfolch
Copy link
Member

dfolch commented Jun 15, 2015

@jlaura If it will make life easier, we can reject this PR (and #624). You can put the network doctest fixes into its own PR (or add them to #628), and then I'll resubmit the topology stuff under a new PR after pulling those fixes.

updates to network.py docstring tests
@sjsrey sjsrey assigned dfolch and unassigned jlaura Jun 17, 2015
@jlaura
Copy link
Member

jlaura commented Jun 17, 2015

As a note - until the open PR for doc-tests is merged, it is going to be necessary to pull this PR locally and manually run the doctests to verify.

@sjsrey
Copy link
Member Author

sjsrey commented Jun 17, 2015

@jlaura we could keep the doctests separate for now, rather than have multiple prs hanging around waiting for another one to get closed. So once you and @dfolch are ok with the network related commits in this pr, this could be merged. Then we can revisit what to do about doctests in the upstream/travis testing. One option for the latter would be to have devs run doctests locally before a pr (as well as the unit tests) but leave the doctests off in travis.

@dfolch
Copy link
Member

dfolch commented Jun 17, 2015

I think it would be safest to turn the doctests back on in travis (once everything is fixed). I talked to @pedrovma and he is good with changes to spreg needed to get all the tests to pass. I'll submit a PR now to fix spreg.

@jlaura let me know if you have any questions about the topology stuff. Here is a zoom-in on major roads in Rhode Island. "Before" (without rounding and removing duplicate segments) and "after". The red dots are what pysal identifies as intersections, the blue lines are actual roads and the grey lines are graph edges.
none_false

11_true

@sjsrey sjsrey mentioned this pull request Jun 17, 2015
@sjsrey sjsrey assigned sjsrey and unassigned dfolch Jun 19, 2015
sjsrey added a commit that referenced this pull request Jun 19, 2015
@sjsrey sjsrey merged commit e475031 into pysal:master Jun 19, 2015
@sjsrey sjsrey deleted the networkrb branch August 17, 2015 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants