Skip to content

Conversation

@ParticularMiner
Copy link
Contributor

Hi @eriknw

As promised, I've just taken my first step and translated from C into python what I judge to be the essential code of FastSV (as found in LAGraph).

Perhaps further modifications can be made to remove unnecessary code. But for now, I'm pleased that it works.

Do please take a look at it and let me know what you think.

By the way, this is my first time using grblas, and so far I'm very impressed by it. I think it's beautiful on so many levels. Well done!

@eriknw
Copy link
Member

eriknw commented Nov 8, 2021

This is really cool to see, @ParticularMiner, and thanks so much for the kind words!

I'm actually on vacation the next week-and-a-half and will be mostly afk and unable to engage. But, I'm really enthusiastic about this and the next steps!

Perhaps @jim22k can take a look at the code and give feedback. We have some new infix notation such as op.min_plus(A @ v) instead of A.mxv(v, op.min_plus), and op.min(x | y) instead of x.ewise_add(y, op.min). Both work, and both are pretty readable imho.

@ParticularMiner
Copy link
Contributor Author

@eriknw

I'm actually on vacation the next week-and-a-half and will be mostly afk and unable to engage.

No problem. Whenever you are ready. Enjoy your vacation!

Copy link
Member

@eriknw eriknw left a comment

Choose a reason for hiding this comment

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

This looks great!

Btw, Tim Davis has been working on CC in LAGraph recently, which may be of interest. See LG_CC_* files here: https://github.com/GraphBLAS/LAGraph/tree/reorg/src/algorithm

I'm curious, was there anything particularly challenging or surprising when using grblas? I know our documentation could be better (and I hope to improve this quarter), but we tried hard to make things interactive with good error messages.

" # flag to terminate FastSV algorithm\n",
" change = Scalar.from_value(True, dtype=bool, name='changed?')\n",
"\n",
" while change.value:\n",
Copy link
Member

Choose a reason for hiding this comment

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

.value is unnecessary here. while change: works.

Copy link
Contributor Author

@ParticularMiner ParticularMiner Nov 30, 2021

Choose a reason for hiding this comment

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

Interesting! You're right, it has a bool() magic method, as my debugger revealed. Cool!

" _, I = f.to_values()\n",
" gp << f[I]\n",
" # Check termination\n",
" mod << op.ne(gp_dup | gp, require_monoid=False)\n",
Copy link
Member

Choose a reason for hiding this comment

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

Is ewise_add necessary here? Can we get by with ewise_mult? Also, there is an isequal helper method for things like this: gb.isequal(gb_dup).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not necessary. I simply had a preference for infix notation but didn't know the infix notation for ewise_mult at that time, or where to find it. So I could only use what you gave me before you left on vacation. Now, looking through the source code, I see that it is something like the following, right?

mod << op.ne(gp_dup & gp)

It seems there's no infix notation for .isequal yet, right?

" n = A.nrows\n",
" I = np.arange(n)\n",
" # The parent of each vertex is initialized to be the vertex itself:\n",
" f = Vector.from_values(I.tolist(), I.tolist(), name='parents')\n",
Copy link
Member

Choose a reason for hiding this comment

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

tolist() is unnecessary. We handle numpy arrays directly (and efficiently).

Copy link
Contributor Author

@ParticularMiner ParticularMiner Nov 30, 2021

Choose a reason for hiding this comment

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

Sure. I only used it because this was an opaque way to use python's default int64 dtype instead of numpy's default int32 dtype on my Windows 10 computer. But that is in itself unnecessary. I'll remove that.

}
],
"source": [
"# grblas.io.draw could do with a few more tunable options to improve pretty display\n",
Copy link
Member

Choose a reason for hiding this comment

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

I totally agree!

@ParticularMiner
Copy link
Contributor Author

Hey @eriknw

You're back! Great. You were missed. Hope you had a good vacation. 😃

I love grblas. Your hard work and expertise is evident as one reads through the source code. I'm learning a lot from your code.

The other day, after consulting Tim Davis, he pointed me to his GxB pack/unpack methods, and I was really surprised to find them (and more) already wrapped in grblas (even though I had to search the source code for them). Yes, I guess that is one thing that surprises me: the depth of exhaustiveness of the package despite the very small number of developers.

I also find grblas is not just a perfunctory python-regurgitation of GraphBLAS. It is a clever reinterpretation of the API. The introduction of the delayed expression object, the infix notation, the use of object attributes in place of GraphBLAS descriptors, and so on, make it a mathematically appealing and intuitive tool that is easy to use. Of course python makes all that possible in a way C/C++ cannot (or at least, not as easily).

I also like the way grblas interfaces with other well-known python modules like scipy.sparse, networkx, and so on. Such features enable data to flow back and forth from grblas seamlessly, without the user having to worry about converting from one data-format to another himself/herself. I haven't yet checked the performance of these conversion routines though. But it would be great if they had O(1) computational complexity wherever possible.

Yes, there are some outstanding issues like the documentation which still needs to expose all the hard work you've done. And I've already pointed out an issue with the pretty display of graphs, which you've already seen. With regard to error messages — strangely, I haven't encountered many. Perhaps that's because it is not easy to make errors with the current syntax. But I'll let you know if I find any incomprehensible ones.

Other issues I hope to bring-up in the future.

Welcome back!

@ParticularMiner
Copy link
Contributor Author

@eriknw

Btw, Tim Davis has been working on CC in LAGraph recently, which may be of interest. See LG_CC_* files here: https://github.com/GraphBLAS/LAGraph/tree/reorg/src/algorithm

Good to know. Will take a close look at it.

"outputs": [],
"source": [
"AA = A.dup()\n",
"AA << P.T @ A @ P"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eriknw
This notebook now contains an example of a compound expression using the matmul operator.

@eriknw
Copy link
Member

eriknw commented Dec 6, 2021

This looks great to me.

Is there anything more you'd like to add before we merge, @ParticularMiner? You're more than welcome to add your name or github handle to the notebook to credit yourself as the author.

@ParticularMiner
Copy link
Contributor Author

Thanks @eriknw

You're more than welcome to add your name or github handle to the notebook to credit yourself as the author.

That's very kind of you. I hadn't thought of that. But since the other notebooks in this folder don't show their author names I prefer not to break with the set "tradition". But if you feel you want to change that now or in the future, just let me know.

Otherwise, I don't think I have anything else to add now. Thanks.

@eriknw
Copy link
Member

eriknw commented Dec 16, 2021

Let's get this in! Thanks @ParticularMiner. We may want to circle around at some point to compare this to the updated algorithms in LAGraph.

@eriknw eriknw merged commit 577084f into python-graphblas:main Dec 16, 2021
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