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

Check if internal J_cols definition is necessary #2173

Conversation

stephane-caron
Copy link
Collaborator

@stephane-caron stephane-caron commented Mar 18, 2024

See #2171

Checking if removing J_cols here breaks a unit test. There are two potential outcomes:

  1. It is necessary and all tests pass, meaning we have an opportunity to add one 😉
  2. It is not necessary, all tests pass, we can double check and merge.

@hrp2-14
Copy link

hrp2-14 commented Mar 18, 2024

Hi ! This project doesn't usually accept pull requests on the main branch.
If this wasn't intentionnal, you can change the base branch of this PR to devel
(No need to close it for that). Best, a bot.

@stephane-caron stephane-caron changed the base branch from master to devel March 18, 2024 10:18
@jcarpent jcarpent closed this Mar 18, 2024
@jcarpent
Copy link
Contributor

The quantity J_cols is exploited in the remainings sweeps of the computMinv of the algorithm.

@stephane-caron
Copy link
Collaborator Author

stephane-caron commented Mar 18, 2024

Ah OK, I hadn't noticed this is a static function. (Woah, those are error-prone.) Sorry for the noise.

I second stated goal of this PR was to check whether this removal breaks any unit test, which I then did locally, and it does break test-cpp-aba 👌

@stephane-caron stephane-caron deleted the feature/check_J_cols_necessity branch March 18, 2024 16:24
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