-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add annotations to FeatureExtractor #761
Conversation
cda9f04
to
6538220
Compare
@vmarkovtsev ping |
Travis failed because of modelforge issues that should be fixed in src-d/modelforge#98 |
@vmarkovtsev all good now? |
format. | ||
|
||
The function is created for backwards capability and should be removed after refactoring is \ | ||
finished. |
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.
The docstring should explain how it is different from file_to_old_parse_file_format
: the input and output types are exactly the same.
78822d7
to
d8793b4
Compare
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.
inspect=lambda span_start, span_stop: self._check_spans_overlap( | ||
start, stop, span_start, span_stop) if start == stop else | ||
span_start <= start and stop <= span_stop) | ||
if start == stop: |
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 is performant ofc, but the following is simpler:
def check_cover(span_start, span_stop):
if start == stop:
return self._check_spans_overlap(start, stop, span_start, span_stop)
return span_start <= start and stop <= span_stop
About #761 (comment) I add a comment. if you read both docstrings you should see the difference. |
I do not see the magic keywords: "This function is different from |
ok, I add it. If not, let me squash all review commits and rebase on current master before the merge. |
Signed-off-by: Konstantin Slavnov <[email protected]>
Signed-off-by: Konstantin Slavnov <[email protected]>
Signed-off-by: Konstantin Slavnov <[email protected]>
Finally! 🎉 |
Now I need to build a new report for the current master. |
This PR adds Annotation tool to use instead of
VirtualNode
class (issue #521).PR contains:
VirtualNode
-sFeatureExtractor
rewritten usingAnnotationManager
.AnnotationManager
back to vnode sequence. They are going to be deleted after we finish refactoring.I also run a regression test to see if there any difference in old vnodes and new (restored) vnodes for quality report.
I did not include the test to the PR. It can be found here: zurk@3aed7d1
I see some improvements like there were a couple of bugs related to column position or mixed intention and the rest is the same. Also, the quality was almost the same for the first 15 repos from the report.
The comparison with v0.2.0 report
I am going to run a full report for the final version after we merge.