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: clean up some auto-generation code #1031
Conversation
Current Aviator status
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.
|
6469a16
to
463d43a
Compare
This pull request failed to merge: some CI status(es) failed. Remove the Failed CI(s): codecov/patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szhorvat: The test behavior hasn't changed in the presence of bug fixes (?). Can you please point me to the relevant test cases in the C core?
@@ -297,7 +297,7 @@ VERTEXSET_LIST: | |||
|
|||
VERTEXSETLIST_INT: | |||
INCONV: | |||
IN: '%I% <- lapply(%I%, function(x) as.integer(x)-1L)' | |||
IN: '%I% <- lapply(%I%, function(x) as.numeric(x)-1)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a mistake on my part. VERTEXSETLIST_INT
is unused and should be completely removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1049
@@ -258,11 +258,11 @@ EDGE_COLOR: | |||
} | |||
} | |||
if (!is.null(%I%)) { | |||
%I% <- as.integer(%I%)-1L | |||
%I% <- as.numeric(%I%)-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. R_SEXP_to_vector_int_copy()
should expect a real, not an integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a test for this:
count_isomorphisms(make_ring(3), make_ring(3), edge.color1=c(2,2,2), edge.color2=c(2,2,2), vertex.color1=c(3,3,3), vertex.color2=c(3,3,3), method='vf2')
This crashed before this fix. Could you please help me by adding this to the test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, it was easy enough. See #1050
This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Remove the Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts). |
463d43a
to
7f2372b
Compare
From #1026.