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

Move more connected components functions to retworkx-core #638

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

Conversation

kris524
Copy link

@kris524 kris524 commented Jul 3, 2022

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2022

CLA assistant check
All committers have signed the CLA.

@kris524 kris524 marked this pull request as draft July 3, 2022 22:51
@kris524
Copy link
Author

kris524 commented Aug 25, 2022

Hello, I get this error no method named 'index' found for reference &<G as GraphBase>::NodeId and I am not sure what other method is allowed. If anyone has any suggestions or has an idea about which files to look at for reference/examples let me know, otherwise I will keep looking.

I am also not sure how to import NullGraph and InvalidNode in the conn_components.rs file.

@kris524 kris524 marked this pull request as ready for review August 26, 2022 07:07
@kris524 kris524 changed the title Draft: Move more connected components functions to retworkx-core Move more connected components functions to retworkx-core Aug 26, 2022
@enavarro51
Copy link
Contributor

I was planning on doing a review of this in the next couple of days. The v.index() issue is that petgraph using Graph , or the derived PyGraph, is different from using the GraphBase trait as it is in rustworkx-core. For instance, NodeIndex for Graph is NodeId for GraphBase. To get the index with GraphBase, you would do graph.to_index(v). You can find to_index here, https://docs.rs/petgraph/0.4.13/petgraph/visit/trait.NodeIndexable.html#tymethod.to_index.

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

There are a number of changes here, but hopefully they are straightforward. There will need to be a couple of tests added and probably a features release note, but those can be done later after you get everything working.

Feel free to ask any questions along the way.

retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
src/connectivity/mod.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/mod.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
retworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
@enavarro51
Copy link
Contributor

Oh one more important thing I forgot. You need to transition to using the rustworkx name. Steps are

  • Rename your repository on Github.
  • Rename your local directory to rustworkx.
  • Change any remote urls to the rustworkx repository.
  • Checkout main on your local.
  • Do a git pull upstream main
  • Move any files you have changed in retworkx-core to the corresponding location in rustworkx-core. I think this will just be in connectivity for conn_components.rs and mod.rs.
  • Once you have moved the files over, you can remove the retworkx-core directory.
  • Finally globally replace references to retworkx in all your changed files to rustworkx.

@kris524
Copy link
Author

kris524 commented Sep 6, 2022

Apologies for the late implementation of the feedback. I have started making the corresponding naming changes and will commit any relevant changes. Thank you for the help!

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

Looks good so far with one change. You should be able to do cargo build without errors on your local branch once you've made the change to rustworkx. You also might want to run cargo fmt in case there are some formatting changes. Once you get a clean build, we can talk about testing.

rustworkx-core/src/connectivity/mod.rs Outdated Show resolved Hide resolved
@enavarro51
Copy link
Contributor

@IvanIsCoding or @mtreinish Can you turn on the workflow testing here? Thanks.

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

A couple more changes should get a clean build.

rustworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
rustworkx-core/src/connectivity/conn_components.rs Outdated Show resolved Hide resolved
@enavarro51
Copy link
Contributor

Hi, @kris524. Just checking to see if you are still working on this. Thanks.

@kris524
Copy link
Author

kris524 commented Oct 11, 2022

Hi @enavarro51 apologies for being slow, I was just busy. Is it ok if I have a look at it this weekend? I think I am close to finishing it.

@enavarro51
Copy link
Contributor

@kris524. That's fine. Once you get a good build, you can add a test for each of the 2 new functions at the end of rustworkx/rustworkx-core/src/connectivity/conn_components.rs, and add a features release note like rustworkx/releasenotes/notes/move-conn_components-functions-1be43732ec6058ea.yaml. Let me know if you have any questions.

@kris524
Copy link
Author

kris524 commented Nov 16, 2022

Tomorrow I will write a test for node_connected_component

@kris524
Copy link
Author

kris524 commented Nov 18, 2022

@enavarro51 Let me know if you are happy with the tests.

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

Successfully merging this pull request may close these issues.

3 participants