-
Notifications
You must be signed in to change notification settings - Fork 72
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 PFD verification #463
Add PFD verification #463
Conversation
49d0d51
to
28f2f04
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.
Please do not fix codestyle in a separate commit.
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.
Overall seems fine, but most of the code just repeats fd_verifier
. As soon as #396 and #458 are merged there will be a task to add afd verification with different measures, which will differ from this pr only in CalculateStatistics
, so directly copying code from fd_verifier
for the 3rd time will look quite bad. I suggest you moving all the common logic of pfd_verifier
and fd_verifier
to a new base class (or at least talk to @chernishev and give this task to someone else). Do it in a separate pr.
error_ = 0; | ||
} | ||
|
||
bool PFDHolds() const { |
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.
Not sure if we need a separate function to check whether pfd holds or not (and store max_fd_error_
) since afd_verifier
just returns the error to the user and pfd is pretty much an afd. But this is a topic to discuss with the maintainers, let me know if you've done it already.
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.
For the future reference: we just talked and decided to fix immediate problems, found by @egshnov. The architecture rework will come later, when both mining and validation PRs for other metrics will be merged.
return clusters_violating_pfd_; | ||
} | ||
|
||
void CalculateStatistics(model::PositionListIndex const* x_pli, |
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.
0cd2e87
to
fea2afd
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.
Please do not make changes to the interface of PfdVerifier
in a separate commit. Amend to the implementation and we are done.
fea2afd
to
97ba170
Compare
97ba170
to
988bbe6
Compare
988bbe6
to
8a3ea79
Compare
8a3ea79
to
e97b3c7
Compare
e97b3c7
to
2a1266a
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.
clang-tidy made some suggestions
c27b3f2
to
ac6093a
Compare
ac6093a
to
f33428f
Compare
f33428f
to
cc09ca4
Compare
This PR implements PFD verification algorithm along with some tests, python bindings and examples.