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

added function torusW #245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

added function torusW #245

wants to merge 2 commits into from

Conversation

MgeeeeK
Copy link
Contributor

@MgeeeeK MgeeeeK commented Feb 13, 2020

  1. You have run tests on this submission, either by using Travis Continuous Integration testing testing or running nosetests on your changes?
  2. This pull request is directed to the pysal/master branch.
  3. This pull introduces new functionality covered by
    docstrings and
    unittests?
  4. You have assigned a
    reviewer
    and added relevant labels
  5. The justification for this PR is: Added an argument torus and its functionality in function lat2w, this fixes the issue pysal/pysal#894

@MgeeeeK MgeeeeK changed the title fixes pysal/pysal#894 fixes for issue pysal/pysal#894 Feb 13, 2020
Copy link
Member

@sjsrey sjsrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for integration and to document expected behaviour.

You can add a test in https://github.com/pysal/libpysal/tree/master/libpysal/weights/tests

See other tests in that directory for examples. Let us know if you have questions after that.

@MgeeeeK
Copy link
Contributor Author

MgeeeeK commented Feb 17, 2020

Please add a test for integration and to document expected behaviour.

I've added a test for the function lat2W(torus=True)

@MgeeeeK MgeeeeK requested review from sjsrey and removed request for ljwolf February 19, 2020 10:49
@MgeeeeK
Copy link
Contributor Author

MgeeeeK commented Feb 20, 2020

@sjsrey should i instead create a new function for torus just like hexLat2W?

@sjsrey
Copy link
Member

sjsrey commented Feb 24, 2020

@MgeeeeK I think separating this out to its own function, maybe torusW would be a good way to modularlize things.

@AppVeyorBot
Copy link

Build libpysal 1.0.25 completed (commit c6a3ba2101 by @sjsrey)

@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@facd17c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #245   +/-   ##
=========================================
  Coverage          ?   81.10%           
=========================================
  Files             ?      116           
  Lines             ?    11633           
  Branches          ?        0           
=========================================
  Hits              ?     9435           
  Misses            ?     2198           
  Partials          ?        0           

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 facd17c...48a2e36. Read the comment docs.

@MgeeeeK MgeeeeK reopened this Feb 24, 2020
@AppVeyorBot
Copy link

Build libpysal 1.0.29 completed (commit 73d5f7a62d by @MgeeeeK)

@MgeeeeK
Copy link
Contributor Author

MgeeeeK commented Feb 24, 2020

@sjsrey new commits contains:

  1. separate function for torus called torusW
  2. unittests for torusW and hexLat2W
  3. fixes for lat2SW (it gave error when (ncols = 1, criterion = "rook") and (ncols = 2, criterion = "queen")

sorry for reopening this pr...i messed up my dev environment earlier so had to drop all my commits.

@MgeeeeK MgeeeeK changed the title fixes for issue pysal/pysal#894 added function torusW Mar 10, 2020
@ljwolf ljwolf added the weights label Oct 19, 2021
@martinfleis martinfleis changed the base branch from master to main February 27, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants