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

refactor: eliminate manual edits from autogenerated files #1207

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

szhorvat
Copy link
Member

@szhorvat szhorvat commented Feb 6, 2024

@krlmlr Can we please have this merged now to reduce the pain of having to untangle manual edits to autogenerated files whenever re-running stimulus?

Issues opened:

Explanation of changes:

  • is_bipartite and realize_bipartite_degree_sequence are new in 0.10.9, hence added due to the C core update
  • has_attribute_table and igraph_version were edited to fix a compiler warning, but we don't actually use them, so removed; issue opened in stimulus
  • get_subisomorphisms_vf2_callback: the generated R code for this was manually removed, and it's simpler to also remove the generated C code until we actually need it.

Ref: #1176

Copy link
Contributor

aviator-app bot commented Feb 6, 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.

@szhorvat
Copy link
Member Author

szhorvat commented Feb 6, 2024

Update: Second commit eliminates the last remaining manual edit. I'm not even sure if this was a manual edit, or the result of a C-side bugfix in the interface definition.

@szhorvat szhorvat force-pushed the refactor/disable-autogen-for-some-functions branch from 53a7505 to 405d633 Compare February 6, 2024 09:51
@szhorvat szhorvat changed the title refactor: reduce number of manual edits to autogenerated files refactor: eliminate manual edits from autogenerated files Feb 6, 2024
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the drift! Please add the "mergequeue" label when good.

Comment on lines -973 to -975
if (igraph_opt("add.vertex.names") && is_named(graph)) {
names(res) <- vertex_attr(graph, "name", V(graph))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Member Author

@szhorvat szhorvat Feb 6, 2024

Choose a reason for hiding this comment

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

This bit of code was incorrect, so I intentionally allowed it to be removed by the auto-generation. This code was trying to add vertex names to a vector that contains edge-quantities, not vertex quantities. I also removed a dependence of the result on V(g) in functions-R.yaml for the same reason, although this seemed to have no ill effect.

If you find anything else that looks suspicious, let me know!

Copy link
Member Author

Choose a reason for hiding this comment

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

Also: As far as I can tell, we're not exposing this function at the moment, so this bug did not have any consequences so far.

as the result is associated with edges and not vertices.
Also fix invalid dependency specification: no dependency on V(g).
@aviator-app aviator-app bot force-pushed the refactor/disable-autogen-for-some-functions branch from 405d633 to 32ef7b3 Compare February 6, 2024 18:14
@aviator-app aviator-app bot merged commit 5a5acb7 into main Feb 6, 2024
12 of 13 checks passed
@aviator-app aviator-app bot deleted the refactor/disable-autogen-for-some-functions branch February 6, 2024 18:45
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.

2 participants