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

Wrong eigenvector_centrality test case in rustworkx-core #798

Closed
IvanIsCoding opened this issue Jan 31, 2023 · 8 comments · Fixed by #872
Closed

Wrong eigenvector_centrality test case in rustworkx-core #798

IvanIsCoding opened this issue Jan 31, 2023 · 8 comments · Fixed by #872
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@IvanIsCoding
Copy link
Collaborator

IvanIsCoding commented Jan 31, 2023

Information

  • rustworkx version: master
  • Python version: 3.10
  • Rust version: 1.61
  • Operating system: Linux

What is the current behavior?

The assert_almost_equal! method in rustworkx-core's test suite for eigenvector_centrality is implemented incorrectly. It always returns true and never panics. This let a bad test case slip through. (see https://github.com/Qiskit/rustworkx/blob/main/rustworkx-core/src/centrality.rs#L433)

What is the expected behavior?

Implement the correct assert_almost_equal! like in #797. After, fix the test case.

Steps to reproduce the problem

Replace any values in the expected_values array and the test case still passes. Implement the correct assert_almost_equal and the test case fails.

@IvanIsCoding IvanIsCoding added bug Something isn't working good first issue Good for newcomers labels Jan 31, 2023
@IvanIsCoding IvanIsCoding changed the title Wrong eigenvector_centraltiy test case in rustworkx-core Wrong eigenvector_centrality test case in rustworkx-core Feb 2, 2023
@mustapha-saad
Copy link

Hi, Can I work on this issue?

@IvanIsCoding
Copy link
Collaborator Author

Hi, Can I work on this issue?

Yes I have assigned the issue to you. You might want to check https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md, it has some tips for your first contribution

@mustapha-saad
Copy link

mustapha-saad commented Feb 28, 2023

Right, thank you. I will go through the codebase right now and create a pull request when I am done...

@mustapha-saad
Copy link

mustapha-saad commented Feb 28, 2023

I can't seem to find eigenvector_centrality test case

Can you please precisely link the folder the issue is here? @IvanIsCoding

@IvanIsCoding
Copy link
Collaborator Author

I can't seem to find eigenvector_centrality test case

Can you please precisely link the folder the issue is here? @IvanIsCoding

The test is in the centrality.rs file, this link takes you to the line defining the incorrect macro used in test cases is: https://github.com/Qiskit/rustworkx/blob/main/rustworkx-core/src/centrality.rs#L433

@mustapha-saad
Copy link

mustapha-saad commented Mar 1, 2023

Okay, thank you.

I am going to do it this way then,

The deliverables would be:
1 - I would separate all the test functions in centrality.rs into its own file in the test folder (following the separation of concern rule),

2 - Then fix the assert_almost_equal test case to test the function correctly...

@mustapha-saad
Copy link

Let me know if i can do it this way so I can start working on it right away...

@IvanIsCoding
Copy link
Collaborator Author

Okay, thank you.

I am going to do it this way then,

The deliverables would be: 1 - I would separate all the test functions in centrality.rs into its own file in the test folder (following the separation of concern rule),

2 - Then fix the assert_almost_equal test case to test the function correctly...

That is correct

mtreinish pushed a commit that referenced this issue May 14, 2023
Closes #798

I generated the values the test case is checking against using NetworkX. So the implementation is correct, it was just the test case and the assert_almost_equal that were incorrect
IvanIsCoding added a commit to IvanIsCoding/rustworkx that referenced this issue May 26, 2023
Closes Qiskit#798

I generated the values the test case is checking against using NetworkX. So the implementation is correct, it was just the test case and the assert_almost_equal that were incorrect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants