Skip to content

Refactor: added invalidateCache() for core cache rebuilding#517

Merged
ZigRazor merged 3 commits intoZigRazor:masterfrom
V1102D:fix-499
Jul 25, 2025
Merged

Refactor: added invalidateCache() for core cache rebuilding#517
ZigRazor merged 3 commits intoZigRazor:masterfrom
V1102D:fix-499

Conversation

@V1102D
Copy link
Copy Markdown
Contributor

@V1102D V1102D commented Jul 18, 2025

Replaced multiple manual calls to cacheAdjMatrix(), cacheDegreeMatrix(), and cacheLaplacianMatrix() with a single invalidateCache() method in Graph.
This improves readability, reduces code duplication, and minimizes the risk of incorrect cache updates after structural graph changes.

No logic changes in caching functions.

Transition matrix is intentionally not included in the centralized method, following issue description.

All 305 unit tests passed successfully after refactoring.

image

@V1102D
Copy link
Copy Markdown
Contributor Author

V1102D commented Jul 21, 2025

Labeler / triage (pull_request_target)

Error: found unexpected type for label 'repo' (should be array of config options)

@ZigRazor
Copy link
Copy Markdown
Owner

Labeler / triage (pull_request_target)

Error: found unexpected type for label 'repo' (should be array of config options)

I open an issue on this.
This is a configuration Error for the CI.

@ZigRazor
Copy link
Copy Markdown
Owner

CI build on windows does not work @V1102D

@V1102D
Copy link
Copy Markdown
Contributor Author

V1102D commented Jul 22, 2025

I would like to clarify an architectural question regarding cache rebuilding.

Currently, I’m considering the following approach to handle the cache update sequence:

void Graph::invalidateCache(bool includeTransitionMatrix) {
cacheAdjMatrix();
cacheDegreeMatrix();
cacheLaplacianMatrix();
if (includeTransitionMatrix) {
cacheTransitionMatrix();
}
}

This introduces a boolean parameter to control whether cacheTransitionMatrix() should be included during cache rebuilding. I understand that the project generally avoids parameters in such functions for simplicity, so I’m unsure whether this approach aligns with the design philosophy.

If adding a parameter is acceptable, I can finalize and push this change. Otherwise, I can revert to sequential function calls or implement a different solution based on your guidance.

I suspect that the current Windows-specific test failure (test_partition) is caused by incomplete cache rebuilding before the transition matrix is generated, but I cannot fully reproduce or verify this issue under Linux.

Please let me know if introducing the parameter in invalidateCache() is acceptable, or if you prefer a different approach.

@ZigRazor
Copy link
Copy Markdown
Owner

I would like to clarify an architectural question regarding cache rebuilding.

Currently, I’m considering the following approach to handle the cache update sequence:

void Graph::invalidateCache(bool includeTransitionMatrix) { cacheAdjMatrix(); cacheDegreeMatrix(); cacheLaplacianMatrix(); if (includeTransitionMatrix) { cacheTransitionMatrix(); } }

This introduces a boolean parameter to control whether cacheTransitionMatrix() should be included during cache rebuilding. I understand that the project generally avoids parameters in such functions for simplicity, so I’m unsure whether this approach aligns with the design philosophy.

If adding a parameter is acceptable, I can finalize and push this change. Otherwise, I can revert to sequential function calls or implement a different solution based on your guidance.

I suspect that the current Windows-specific test failure (test_partition) is caused by incomplete cache rebuilding before the transition matrix is generated, but I cannot fully reproduce or verify this issue under Linux.

Please let me know if introducing the parameter in invalidateCache() is acceptable, or if you prefer a different approach.

yes, it's acceptable

@ZigRazor ZigRazor merged commit ac193ea into ZigRazor:master Jul 25, 2025
27 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants