-
Notifications
You must be signed in to change notification settings - Fork 117
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
Defining pow functions for adjacency and transition matrices #463
base: master
Are you sure you want to change the base?
Conversation
namespace CXXGraph { | ||
|
||
template <typename T> | ||
const PowAdjResult Graph<T>::powAdjMatrix(unsigned int k) 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.
Why two specific methods? I think that one method overloaded two times would be better.
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.
Okay! I'll just have the user pass an enum value to distinguish. Or, would you prefer they pass the actual matrices themselves to the function? I just want to make sure I'm being consistent with the rest of the repo's style.
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.
virtual const PowAdjResult matrixPow(unsigned int k, AdjacencyMatrix adj) const
with usage like:
graph.matrixPow(2, graph.getAdjMatrix());
This feels wrong to me... should I make a function w/ is not a member of the CXXGraph class? That also feels wrong...
What kind of overloading are you thinking?
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.
Hi, sorry for the delay, I'm very busy these weeks. For me the best thing would be to implement them as free functions, not as Graph methods. But we can hear what @ZigRazor thinks.
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.
Sounds good! No worries!
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.
Hey @ZigRazor
Just following up here - would like to get this wrapped up soon! Thoughts on implementing this as a free function?
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.
Sorry for the delay.
Yes, the best choice for me is to implement them as a free functions.
Thank you
std::vector<std::vector<T>> res(n, std::vector<T>(n, 0)); | ||
|
||
// build identity matrix | ||
for (int i = 0; i < n; i++) res[i][i] = 1; |
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.
I would use a standard algorithm for this, like std::transform
, std::fill
or std::generate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #463 +/- ##
==========================================
+ Coverage 97.87% 97.89% +0.02%
==========================================
Files 87 89 +2
Lines 10079 10274 +195
Branches 670 687 +17
==========================================
+ Hits 9865 10058 +193
- Misses 214 216 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
#455 - Define pow function for graph matrices
powAdjMatrix
andpowTransitionMatrix
, respectively).@ZigRazor @sbaldu