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

libpysal.graph roadmap to release #562

Open
26 of 29 tasks
martinfleis opened this issue Sep 7, 2023 · 28 comments
Open
26 of 29 tasks

libpysal.graph roadmap to release #562

martinfleis opened this issue Sep 7, 2023 · 28 comments
Assignees
Labels

Comments

@martinfleis
Copy link
Member

martinfleis commented Sep 7, 2023

I tried to outline what is missing before we could cut a release with the new graph stuff. Open to a discussion.

testing

We have a decent coverage of base but especially constructors are not always tested for correctness. So we test the API but not that the adjacency captures what it should. So missing tests:

  • test correctness of contiguity builders finalise contiguity graph builders #566
  • test correctness of triangulation builders
  • test correctness of kernel builders
  • test all kernel parameters
  • test all query options (both sklearn and scipy)
  • test precomputed distance matrix support (indirectly via triangulation and distance band already covered)

implementation/checks

Some bits are still waiting for implementation (I have likely missed stuff here) or a double check that the current implementation works as intended.

  • isolates in kernel and knn
  • co-located in knn
  • knn haversine checks
  • fix cliques
  • set operations
  • draft API and raise NotImplementedError?

discussion

A few things I'd like to discuss (during the dev call in 3 minutes?)

  • default kernel - W has triangular, we now have gaussian following scipy model - do we want to warn? how?
  • expose kernels in transform? kernels are essentially transformations. Shall we allow to use them post-creation?
  • how should set operations work? See perimeter weights, fix kernel, set_ops #561

follow-up

Stuff that is not implemented or started but that can likely wait for subsequent release, not the initial one. Below is the list of stuff I'd like to see soon but there's more to be done.

documentation

API reference should be in the docs for the first release. user guide can follow later.

  • API reference
  • user guide
  • migration guide

Some of the follow-up could be piped via W for the time being if we want them available.

@martinfleis
Copy link
Member Author

Can you drop a note if you are planning to work on any of the topics mentioned above so we don't overlap?

@ljwolf
Copy link
Member

ljwolf commented Sep 13, 2023

I am unlikely to have time until sept 27

@knaaptime
Copy link
Member

I think I’ll go back to ensuring the constructors can consume sparse next

@martinfleis
Copy link
Member Author

With #577, we're ready for the initial experimental release of Graph.

  • cliques are not fixed, but they raise NotImplementedError at the moment.
  • precomputed matrix is not tested explicitly but a few of our internal functions use it, so it is tested via those.
  • API is not yet drafted, but that does not need to be for a release
  • The default kernel is still Gaussian
  • set operations are implemented with some restrictions ensuring the validity of the Graph and its alignment with original data. See implement set ops and to_networkx #575 for details.

Anything else you'd like to get in?

I would target the end of the week at the latest to cut 4.8.0 (need it alive on conda on Tuesday)

@martinfleis
Copy link
Member Author

Anything blocking the release now that #577 is merged?

@jGaboardi any idea if the current infrastructure works as intended? It has not been updated here.

@jGaboardi
Copy link
Member

I am not sure. Let's give it try; perhaps as a 4.7.0.post1 release?

@martinfleis
Copy link
Member Author

martinfleis commented Sep 29, 2023

More like 4.8.0rc1 given the status of current main

@martinfleis
Copy link
Member Author

@martinfleis
Copy link
Member Author

Documentation is okay - https://pysal.org/libpysal/

@jGaboardi
Copy link
Member

Changelog is completely broken

Yeah, looks like the action still uses tools/gitcount.ipynb. This will need to be looked it if we still want to use it.

spaghetti has been using actions/github-script@v6 with success, if we want to try here.

@knaaptime
Copy link
Member

lets update the root and get rid of versioneer etc before doing a real release

@martinfleis
Copy link
Member Author

@knaaptime Are you on it or shall I?

@knaaptime
Copy link
Member

i'll take the first pass

@martinfleis
Copy link
Member Author

Unless anyone does that first, I'm happy to cut 4.8.0 later tonight (Prague time) or tomorrow morning.

@jGaboardi
Copy link
Member

Unless anyone does that first, I'm happy to cut 4.8.0 later tonight (Prague time) or tomorrow morning.

Seem to have a strange merge problem with upstream/main vs. my main. Both docs build and publish failed. Going to delete the v4.8.0 tag and try to figure it out.

@jGaboardi
Copy link
Member

Seems like two key pieces slipped through the cracks:

  1. Updated method for install deps for creating release.
  2. This line seems to be missing in libpysal/docs/conf.py

I'll put in the necessary PR shortly.

@knaaptime
Copy link
Member

we dont need the sys path hack

@knaaptime
Copy link
Member

i think we're using an old recipe for the docs. It should have an install line before making the docs (so you get the real version without doing the sys/path hack). I think there's a current version in tobler

@jGaboardi
Copy link
Member

jGaboardi commented Oct 1, 2023

we dont need the sys path hack

Seems to fail without it. Is there something I am missing? Yes, there was. LOL

@jGaboardi
Copy link
Member

@knaaptime @martinfleis

Can confirm docs build locally, but with the following warnings:

libpysal/libpysal/cg/shapes.py:docstring of libpysal.cg.LineSegment.bounding_box:1:<autosummary>:1: WARNING: Inline strong start-string without end-string.
libpysal/libpysal/cg/shapes.py:docstring of libpysal.cg.LineSegment.bounding_box:1:<autosummary>:1: WARNING: Inline strong start-string without end-string.
libpysal/docs/notebooks/fetch.ipynb:4: WARNING: Each notebook should have at least one section title
libpysal/docs/notebooks/io.ipynb:4: WARNING: Each notebook should have at least one section title
libpysal/docs/notebooks/Raster_awareness_API.ipynb: WARNING: document isn't included in any toctree
libpysal/docs/notebooks/fetch.ipynb: WARNING: document isn't included in any toctree
libpysal/docs/notebooks/io.ipynb: WARNING: document isn't included in any toctree
  • The first two, I can't find anything to actually fix after finding some possible solution.
  • The second two are "whatevers".
  • For the final three, are we not including those in tutorial.rst on purpose? If so, then nothing to fix.

@jGaboardi
Copy link
Member

I we are OK with that, I will push up the fixes immediately.

@jGaboardi
Copy link
Member

Release failed, I think because there is no README.md in libpysal... it's README.rst.

@jGaboardi
Copy link
Member

@knaaptime @martinfleis

v4.8.0rc2 is tagged and up on PyPI with docs built successfully. Please give a once over. If everything looks kosher, I will cut v4.8.0.

@martinfleis
Copy link
Member Author

All looking good to me!

@jGaboardi
Copy link
Member

Do you or Eli want to do the honors since yall put in that hard work? Or shall I go ahead and cut it?

@martinfleis
Copy link
Member Author

Go ahead 😉

@martinfleis
Copy link
Member Author

The decision on ID API has been to allow passing an array of IDs when the input is a sparse or dense precomputed array and raise a ValueError when it is passed alongside a data frame.

@jGaboardi
Copy link
Member

xref #712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants