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

Feat: Detailed Statistics #526

Merged
merged 2 commits into from
Aug 1, 2023
Merged

Feat: Detailed Statistics #526

merged 2 commits into from
Aug 1, 2023

Conversation

umut-sahin
Copy link
Contributor

No description provided.

@umut-sahin umut-sahin requested a review from youben11 July 26, 2023 15:05
@cla-bot cla-bot bot added the cla-signed label Jul 26, 2023
@umut-sahin umut-sahin force-pushed the feat/detailed-statistics branch 21 times, most recently from 6d7e0e6 to 309fa5f Compare August 1, 2023 07:14
@umut-sahin umut-sahin force-pushed the feat/detailed-statistics branch 2 times, most recently from 90804b9 to b4ab47a Compare August 1, 2023 08:28
@umut-sahin umut-sahin marked this pull request as ready for review August 1, 2023 09:37
Copy link
Member

@youben11 youben11 left a comment

Choose a reason for hiding this comment

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

I don't see why querying stats (e.g. to get the count of an operation per tag), is implemented in Python, and at the CP level. This is a feature that should be implemented in Cpp in the CompilerFeedback class (thus available to be supported in the Rust bindings or the CAPI), and exposed through the python bindings to CP.

@umut-sahin
Copy link
Contributor Author

Because it's much easier to implement, and we have a lot more things to do. We can move it to C++ when it's beneficial to do so.

Also, I'd much prefer to replicate this logic (which is 4 function at core) in Rust, than to do it in C++. As for the CAPI, we can't return a hashmap there anyway.

@youben11
Copy link
Member

youben11 commented Aug 1, 2023

Because it's much easier to implement, and we have a lot more things to do. We can move it to C++ when it's beneficial to do so.

Also, I'd much prefer to replicate this logic (which is 4 function at core) in Rust, than to do it in C++. As for the CAPI, we can't return a hashmap there anyway.

While I'm not a fan of adding this outside of the Compiler Cpp codebase, the only way I would see this be kept outside of Cpp, is this feature being implemented under concrete.compiler.CompilerFeedback

cc @BourgerieQuentin

@umut-sahin
Copy link
Contributor Author

Done.

@umut-sahin umut-sahin merged commit ade83d5 into main Aug 1, 2023
@umut-sahin umut-sahin deleted the feat/detailed-statistics branch August 1, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants