Skip to content

fix: biconnected_components() now returns edge and vertex sequences again#1213

Merged
aviator-app[bot] merged 6 commits intomainfrom
fix/biconnected_components-2
Feb 8, 2024
Merged

fix: biconnected_components() now returns edge and vertex sequences again#1213
aviator-app[bot] merged 6 commits intomainfrom
fix/biconnected_components-2

Conversation

@szhorvat
Copy link
Copy Markdown
Member

@szhorvat szhorvat commented Feb 7, 2024

This is a temporary workaround for #1203 until auto-generation is updated to handle this situation. It restores the 1.2.11 behaviour.

@krlmlr I recommend this for the next bugfix release, but we'll have to find a proper solution later, with auto-generation.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Feb 7, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@krlmlr krlmlr changed the title fix: biconnected_components() now returns edge and vertex sequences again fix: biconnected_components() now returns edge and vertex sequences again Feb 8, 2024
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 8, 2024

Thanks. We want to move away from using .Call() outside aaa-auto.R . Can we achieve the same by merely wrapping biconnected_components_impl() instead of killing it?

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Feb 8, 2024

I can't, as in I don't know how. The generated version is buggy. This would require a change to stimulus. I could have patched aaa-auto.R directly, but that causes a lot of inconvenience.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 8, 2024

Can you please add a test? Happy to propose something.

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Feb 8, 2024

I won't have time for a test at the moment, but since this is an obvious bugfix, it would be good to get it out soon ...

Of course eventually we'll need a test. If we had one, this breakage would have been detected.

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Feb 8, 2024

@krlmlr There is already a test for this function. What we need to do is make sure that the components of the result list are named as expected, and are of the correct class (igraph.vs and not numeric). How do you do this? Just check names() and class()?

@szhorvat szhorvat force-pushed the fix/biconnected_components-2 branch 2 times, most recently from 9e85cfc to c881b0d Compare February 8, 2024 18:59
@szhorvat szhorvat requested a review from krlmlr February 8, 2024 19:00
…gain

This is s temporary workaround for #1203 until auto-generation is updated to handle this situation.
@krlmlr krlmlr force-pushed the fix/biconnected_components-2 branch from c881b0d to 38d68e3 Compare February 8, 2024 19:08
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 8, 2024

5556282 is what I mean by wrapping. I believe there's always a way, our autogeneration is not that bad.

The other commits are cosmetics and add a test.

@aviator-app aviator-app bot merged commit ddc8c50 into main Feb 8, 2024
@aviator-app aviator-app bot deleted the fix/biconnected_components-2 branch February 8, 2024 20:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants