@ljwolf do you think it would make sense to move spenc into spopt rather than maintaining it as a standalone package?

I cant seem to find a public record, but I've discussed with @ljwolf that spopt would be a preferable home for spenc instead of a standalone package. This issue serves as a reminder to move that code over here (replaces #80)

raising for discussion: is it worth being explicit in the API about clustering algos that strictly regionalize (e.g. maxp, skater) vs those that blend geographic and attribute dissimilarities (e.g. spenc, spatial_ward, etc) to get a spatially-encouraged solution? Might we want to put them in separate submodules or something to make it more explicit to the user?

spenc is not defined in requirements.txt but is used in spopt/region/spenc.py.

• Rename doc to docsrc (also see #40)
• remove readthedocs.yml
• other?

Request inclusion for GitHub Discussions.

See pysal/spaghetti#548

The stub for https://github.com/pysal/spopt/blob/master/spopt/BCMaxP.py#L210 is using a general spatial distance rather than contiguity. We need to redesign the API to clarify what type of matrix is required for the different problems we will implement

When I ran sphinx-build -b html docsrc docs to build docs website, I got

Running Sphinx v3.1.2
Configuration error:
There is a programmable error in your configuration file:
Traceback (most recent call last):
File "/Users/anaconda3/lib/python3.8/site-packages/sphinx/config.py", line 319, in eval_configfile
execfile(filename, namespace)
File "/Users/anaconda3/lib/python3.8/site-packages/sphinx/util/pycompat.py", line 88, in execfile_
exec(code, _globals)
File "/Users/xinfeng/spopt_rst/docsrc/conf.py", line 60, in <module>
version = spopt.version
AttributeError: module 'spopt' has no attribute 'version'

However, the first line of the init.py under the folder spopt is:
__version__ = "0.1.0"

I have tried to type import spopt and spopt.__version__ in python, and get the right answer. NOT sure why conf.py cannot obtain the version info via import spopt.

• Contributor Guidelines (see #42)
• Code of Conduct (link to PySAL)
• Project Governance (link to PySAL)

The PySAL banner (pysal_banner.svg) was deleted from the repo in #86, as well as the .ico image being swapped out for the old PySAL one. Right now we need to add that back. In the the future, it is important to perform code review as often and as thoroughly as possible to prevent this from happening, especially with PRs touching 172 files. I am always available for code review.

#18 #49

Following the 2020-08-14 dev meeting, it was decided that all solvers APIs will be conditional dependencies and will be available through a solver.py module. @sjsrey, can you provide a description and example on how that can be implemented?

Further, the distinction was clarified between a solver and a solver API, with examples of the former being CBC and Gurobi, and examples of the latter being PuLP and Python-MIP.

There is an ongoing issue with PuLP for Python 3.8 on conda-forge.

See oturns/geosnap#241 and conda-forge/pulp-feedstock#4

There is an ongoing issue leading to the following errors.
Error: Unable to process command '::add-path::/usr/share/miniconda3/condabin' successfully.
Error: Unable to process command '::set-env name=CONDA::/usr/share/miniconda3' successfully.
Error: The add-path command is disabled.
Error: The set-env command is disabled.

• Python 3.5, 3.6 --> Python 3.6, 3.7

• add requirements_pip.txt to setup_package()

• use package variable for name

• add GitHub url

We should add the information for the current source to the Funding section. I put a template up of entries here that are commented out:

<!-- [<img align="middle" src="docs/_static/image/IMAGE1.png" width="150">](link1) Some text1: [Project title 1](another_link1) -->

Once we have the needed info. (and image, links, etc.), we can simply remove the comment markers (<!-- and -->). As an finished example, see spaghetti's Funding section.

Google's ortools is used in BaseClass.py, but it is not listed in requirements.txt leading to a failure in TravisCI:

======================================================================
ERROR: Failure: ImportError (No module named 'ortools')

The current implementation of BaseSpOptExactSolver sets the solver to the ortools wrapper for CBC: pywraplp.Solver. In order to make spopt child model classes solver agnostic should we remove the declaration of self.spOptSolver here and have self.solve as an @abstractmethod like in BaseSpOptHeuristicSolver?

Related: #18

Review:

• Geosnap
• Region
• Other

Document these lists on the wiki page

Should spopt adopt nose, pytest, or another model for performing tests?

comment 1

comment 2

There is a formatting error on the Installation page.

• Deadline Nov 13

https://www.tandfonline.com/doi/pdf/10.1080/13658816.2020.1808221 interesting new method we may want to implement

@knaaptime Why was ci/ changed to .ci/ in #95?

See here

The failure is cropping up in this PR and other PR.

I just wanted to put this here for a reminder. There is a new(ish) Python solver package that we may condsider: coin-or/python-mip.

Since we are (kind of) starting from scratch, should we decide on black formatting for code.

See point 2 in the spaghetti contributor guidelines.

• setup.py needs to be updated to conform with the submodule_template

• Maintainer info in setup.py needs updating

  • Currently:
    • maintainer="PySAL Developers"
    • maintainer_email='[email protected]'
  • Should be updated once the new maintainers are decided for region.

.travis.yml does not conform with the submodule_template .

1. Renaming the organization remote master branch.

1. Create a renamed copy of the master branch.

•  $ git branch -m master main
  

2. Push the renamed copy to the organization remote.

•  $ git push -u upstream main
  

3. Update the default branch in Settings/Branches.
4. Delete the organization remote master.

•  $ git push upstream --delete master
  

2. Renaming your remote master branch.

1. Create a renamed copy of the master branch.

•  $ git branch -m master main
  

2. Push the renamed copy to your remote.

•  $ git push -u origin main
  

3. Update the default branch in Settings/Branches.
4. Delete your remote master.

•  $ git push origin --delete master
  

• create a pysal/spopt gitter channel
• add the following language in the Support section of README.md:
  • "If you are having trouble, please create an issue or talk to us in the gitter room."

There are multiple headings that do not have any text.

I'll open this issue so we can brainstorm some ideas for topics under the "Manage topics" headline.

https://github.com/pysal/pysal/blob/8612af9f83cef755baa9e040222a7c200073690a/pysal/cg/rtree.py#L461

We are doing a max of a tuple with first element as a float (silhoutte coefft) and second element as a _NodeCursor. Generally, this works fine if the first element is different. However, there are corner cases when silhoutte for two clusters are exactly the same. At this point the second element of the tuple (_NodeCursor) is compared. And, ofcourse, comparison operators (>,<) are not valid for the class _NodeCursor. Hence it throws an error:

...
File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/pysal/cg/rtree.py", line 200, in insert
    self.cursor.insert(o, orect)
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/pysal/cg/rtree.py", line 417, in insert
    self._balance()
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/pysal/cg/rtree.py", line 462, in _balance
    [(silhouette_coeff(c, memo), c) for c in clusterings])
TypeError: '>' not supported between instances of '_NodeCursor' and '_NodeCursor'

Fix:
Take the argmax of silhoutte score, and choose one cluster based on the index:
Line number: 462: /Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/pysal/cg/rtree.py

Replace below original code:

score, bestcluster = max(
  [(silhouette_coeff(c, memo), c) for c in clusterings])

by the below code

import numpy as np
#print([(silhouette_coeff(c, memo), c) for c in clusterings])
sil_scores = [silhouette_coeff(c, memo) for c in clusterings]
best_sil_index = np.argmax(sil_scores)
bestcluster = clusterings[best_sil_index]
score = sil_scores[best_sil_index]

• Add sections like other PySAL submodules
  • pysal logo
  • quick blurb
  • Examples
  • Installation
  • Requirements
  • Contribute
  • Support
  • CoC
  • License
• Update subtitle -- Regionalization, facility location, and transport-oriented modeling
• Redo MaxP notebook screenshot
  • python code chunk in markdown
  • high-res retina image of regions
• Rename Location Allocation subsection to Facility Location
• Create new subsection: Transportation and Routing

• descartes is currently needed for to run the binders
  • we can't plot polygons without it
  • this will not be needed from geopandas v0.9.0 (I think), but must be there for now
• we should have a binder badge in the Examples section of README.md

test all binders prior to closing

Following up with #2, maybe we should reconsider the structure of spopt. The current structure (of non-base classes, etc) is:

• allocate
• region
• route

I propose we change allocate to facility_location (or facloc for shorthand). I intend to add in covering models (LSCP, MCLP, etc.) and those don't generally fall under "location allocation". Also, I would like to add in a Transportation Problem class, so maybe changing route to transport, which would house transportation-oriented models, is a good idea. So the new structure would be:

• facloc
• region
• transport

The if condition is always false here, the label could never be assigned -1

Hi! I'm using google's Colab but struggling to install spopt, specifically looking to use MaxPHeuristic.

Any ideas why I'm unable to install spopt in Colab?

Creating issue for this comment

we are generating a cov report, but not actually checking/comparing/reporting anything from it. If we would like to set up codecov, I can do that easily, I will just need admin access to spopt.

See example in spaghetti

@sjsrey

• rename LICENSE -> LICENSE.txt 61353f8
• add a MANIFEST.in file 267753c
• update supported Python versions (+3.9, -3.6)
  • setup.py (also python_requires=">=3.7") 21f963e
  • installation.rst 75a77b1
• update binder link creation in conf.py and DRY in general 89631cd
• remove pin spaces in .ci/*.yml files c958bf4
• reduce min builds before reporting in codecov.yml 3f6264f
• adjust environment.yml f788d0b
  • bump Python version to 3.9, -3.8
  • replace coveralls with codecov
• requirements*.txt files
  • alphabetize 89a1234
  • create requirements_tests.txt with pytest, pytest-cov, & codecov a10dbfc
  • create requirements_dev.txt with black & pre-commit c2da3e4
• update setup.py 21f963e
  • add keyword in setup(): py_modules=[package]
  • add dev & testing reqs to _groups_files

Deadline December 11

• Documentation
  • docsrc/ and docs/ directories
  • mimic docsrc/ dir of other PySAL submodules
    • conf.py
    • Makefile
    • _static/
    • index.rst/installation.rst/api.rst/tutorials.rst/references.rst
• Testing
  • locate
  • region
  • transport

I'm not running checks on the input, nor casting input to an array in the Spanning_Tree clusterer.

update travis CI testing conventions based on guidance from @ocefpaf

@knaaptime

The failure due to the coverage decrease can be ignored... for now. However, we are desperately in need of a more complete/robust testing suite. At the moment only half our code base is being tested, which paves the way for undetected bugs.

See here for my original comment.

region_k_means.py includes several imports that are not used (lines 2,3):

from libpysal.weights.util import get_points_array_from_shapefile
from libpysal.io.fileio import FileIO as psopen

Also, defining neighboring within is_neighbor() (line 66) does not appear to be necessary:

def is_neighbor(area, region, w):
    """Check if area is a neighbor of any member of region"""
    neighboring = False
    for member in region:
        if area in w[member]:
            return True
    return False

@rwei5 do we want to keep https://github.com/pysal/spopt/blob/master/spopt/region/BCMaxP.py or is now redundant?

They are good examples, but the new user might benefit from more narrative.

• Deadline July 10
• Follows after #33

There are several errors in the README.md that was just merged. It's best to not merge directly to main and to have another person review PRs for this reason.

• In the Requirements section
  • the pandas link points to the libpysal docs
  • the libpysal link points to the spaghetti docs
  • scikit-learn is misspelled

As per our meeting this morning, here is a class I wrote for my dissertation which uses pulp for solving four fundamental facility location problems (LSCP, MCLP, PMP, PCP).

pysal/region#18