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

167 largest cycle in statistics not being computed correctly #176

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dracpak
Copy link
Collaborator

@dracpak dracpak commented Nov 11, 2024

Fixes #167

What was changed?

The data in the right sidebar was updated and fixed so that the statistics are correct.

Why was it changed?

Currently, some of the stats on the right side of the page are incorrect, as they either reference the wrong data or are parsed incorrectly. This may give users the wrong impression about the functions they are looking at and they may draw incorrect assumptions. The fix changes this so now the stats are correct for the returned functions, allowing for quick analysis.

How was it changed?

The postgres_connector.py file was changed. Specifically, the get_statistics() function was updated and altered so that the values that get pushed to the front end is the data that should be getting sent. There was no need to change any functionality on the frontend as all of the fields were present.

@dracpak dracpak linked an issue Nov 11, 2024 that may be closed by this pull request
2 tasks
@iamsyedomair
Copy link
Collaborator

Overall, this looks like a solid implementation that fixes the core issues. The code is clean and follows good practices. Just a few suggestions that might make it even more robust, the JOIN operation in the periodic_cycles query might benefit from an index on graph_id if one doesn't already exist. For large datasets, we might want to ensure this query is optimized. but apart from that clear SQL query structure for calculating periodic cycles - the separation into a distinct SELECT statement makes the logic easier to follow and maintain and the code follows the existing codebase style and patterns.

@suprajamannava17
Copy link
Collaborator

The PR performs well for the largest cycles and resolves the preperiodic cardinality, aligning with the feature’s objectives. For refinement, since the cardinality field already exists, consider utilizing it directly rather than recalculating sums as seen in "preperiodic_cardinalities = [sum(comp) for comp in preperiodic_components]". Additionally, I noticed duplicates in the automorphism group cardinality for value 2; addressing these will ensure data accuracy. This work shows solid progress—keep up the great work, and with these final tweaks, we’ll be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

largest cycle in statistics not being computed correctly
3 participants