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

wasm collaboration network first pass #228

Merged
merged 11 commits into from
Sep 29, 2023

Conversation

JamesKunstle
Copy link
Collaborator

adds utility of joining two or more repo networks in the context of some general WASM projects, suggests next steps for improvement.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:41Z
----------------------------------------------------------------

How/why did you choose the set of repos? Was 5 a specific choice?


JamesKunstle commented on 2023-09-19T14:44:58Z
----------------------------------------------------------------

No it was arbitrary. This notebook isn't really trying to be experimental, just demonstrate the technique of combining networks, so I picked a few preeminent projects within scope.

cdolfi commented on 2023-09-20T17:52:24Z
----------------------------------------------------------------

got it

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:43Z
----------------------------------------------------------------

What does a node represent here?


JamesKunstle commented on 2023-09-19T14:45:24Z
----------------------------------------------------------------

++ need to clarify this.

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:44Z
----------------------------------------------------------------

the


JamesKunstle commented on 2023-09-19T14:45:34Z
----------------------------------------------------------------

++

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:45Z
----------------------------------------------------------------

Line #3.        ("wasmtime", "bytecodealliance"),

Why is wasi not included?


JamesKunstle commented on 2023-09-19T14:46:14Z
----------------------------------------------------------------

wasi was failing for some reason so I just removed it for consideration.

cdolfi commented on 2023-09-20T17:54:18Z
----------------------------------------------------------------

Makes sense, lets open an issue to look into this to see why/ make sure there is nothing bigger happening w that repos data

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:45Z
----------------------------------------------------------------

Plotly?


JamesKunstle commented on 2023-09-19T14:46:51Z
----------------------------------------------------------------

Yeah the backend draws in Plotly.

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:47Z
----------------------------------------------------------------

same comment for the image below, also can the legend be included in the photo?


JamesKunstle commented on 2023-09-19T14:47:23Z
----------------------------------------------------------------

++

cdolfi commented on 2023-09-20T17:55:09Z
----------------------------------------------------------------

Can you add the legend to the picture?

JamesKunstle commented on 2023-09-20T18:28:44Z
----------------------------------------------------------------

Adding a legend is actually a really complicated process for this many nodes, unfortunately, so I'd rather make a note and merge this PR than spend the time hacking the legend in- are you amenable to that?

cdolfi commented on 2023-09-20T19:03:11Z
----------------------------------------------------------------

Sounds good to me!

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 14, 2023

View / edit / reply to this conversation on ReviewNB

cdolfi commented on 2023-09-14T14:51:48Z
----------------------------------------------------------------

Can you import the photos into the notebook so they are shown ? Also include the legend, not sure exactly what I am looking at

example from the key_mgmt notebook

img = 'keylime_company.png'
Image(filename=img)

JamesKunstle commented on 2023-09-19T14:47:13Z
----------------------------------------------------------------

++ Yeah definitely

Copy link
Collaborator Author

No it was arbitrary. This notebook isn't really trying to be experimental, just demonstrate the technique of combining networks, so I picked a few preeminent projects within scope.


View entire conversation on ReviewNB

Copy link
Collaborator Author

++ need to clarify this.


View entire conversation on ReviewNB

Copy link
Collaborator Author

++


View entire conversation on ReviewNB

Copy link
Collaborator Author

wasi was failing for some reason so I just removed it for consideration.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Yeah the backend draws in Plotly.


View entire conversation on ReviewNB

Copy link
Collaborator Author

++ Yeah definitely


View entire conversation on ReviewNB

Copy link
Collaborator Author

++


View entire conversation on ReviewNB

Signed-off-by: James Kunstle <[email protected]>
@JamesKunstle
Copy link
Collaborator Author

@cdolfi updated PR

Copy link
Contributor

cdolfi commented Sep 20, 2023

got it


View entire conversation on ReviewNB

Copy link
Contributor

cdolfi commented Sep 20, 2023

Makes sense, lets open an issue to look into this to see why/ make sure there is nothing bigger happening w that repos data


View entire conversation on ReviewNB

Copy link
Contributor

cdolfi commented Sep 20, 2023

Can you add the legend to the picture?


View entire conversation on ReviewNB

@cdolfi
Copy link
Contributor

cdolfi commented Sep 20, 2023

@JamesKunstle only small comments left on my side. @hemajv @oindrillac would yall be able to review as well?

Copy link
Collaborator Author

Adding a legend is actually a really complicated process for this many nodes, unfortunately, so I'd rather make a note and merge this PR than spend the time hacking the legend in- are you amenable to that?


View entire conversation on ReviewNB

Copy link
Contributor

cdolfi commented Sep 20, 2023

Sounds good to me!


View entire conversation on ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 21, 2023

View / edit / reply to this conversation on ReviewNB

oindrillac commented on 2023-09-21T13:08:22Z
----------------------------------------------------------------

Do the edges represent an existence of a relationship or are they also weighted by the number of contributions? I think both are significant. If it is the former, maybe we want to also explore the latter at some point?


hemajv commented on 2023-09-21T20:05:00Z
----------------------------------------------------------------

How are we defining the communities here?

JamesKunstle commented on 2023-09-22T20:54:21Z
----------------------------------------------------------------

replaced 'community' with 'repo'

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 21, 2023

View / edit / reply to this conversation on ReviewNB

oindrillac commented on 2023-09-21T13:08:23Z
----------------------------------------------------------------

This is awesome work! Thank you @JamesKunstle, @mariashev

I know that this work is not tied to a hypothesis, I would like us to "prove out this hypothesis" at some point, by running this on a possibly known set of linchpin contributors - I describe an example here https://github.com//issues/200 It would be interesting to identify that list of critical members algorithmically and also contributors who are beyond this list but serve as linchpins.


JamesKunstle commented on 2023-09-22T20:54:44Z
----------------------------------------------------------------

++ sounds great

Copy link
Contributor

hemajv commented Sep 21, 2023

How are we defining the "communities" here?


View entire conversation on ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 21, 2023

View / edit / reply to this conversation on ReviewNB

hemajv commented on 2023-09-21T20:08:49Z
----------------------------------------------------------------

Small typo: We merge that collaboration netowork... change to network

If I understand correctly, after merging the networks the final network should have contributors as the nodes right?


JamesKunstle commented on 2023-09-22T20:45:20Z
----------------------------------------------------------------

Yes exactly. The nodes of the merged network are the union of the nodes of the two networks.

@review-notebook-app
Copy link

review-notebook-app bot commented Sep 21, 2023

View / edit / reply to this conversation on ReviewNB

hemajv commented on 2023-09-21T20:08:50Z
----------------------------------------------------------------

Can you also include a brief markdown explaining what this function does?


@review-notebook-app
Copy link

review-notebook-app bot commented Sep 21, 2023

View / edit / reply to this conversation on ReviewNB

hemajv commented on 2023-09-21T20:08:51Z
----------------------------------------------------------------

any reason we chose the start year as 2017? Perhaps, worthwhile to mention it here as well


@hemajv
Copy link
Contributor

hemajv commented Sep 21, 2023

@JamesKunstle @mariashev great work on the collaboration network analysis 🎉
@JamesKunstle I have reviewed the notebook and left a few comments

Copy link
Collaborator Author

Yes exactly. The nodes of the merged network are the union of the nodes of the two networks.


View entire conversation on ReviewNB

Copy link
Collaborator Author

replaced 'community' with 'repo'


View entire conversation on ReviewNB

Copy link
Collaborator Author

++ sounds great


View entire conversation on ReviewNB

@JamesKunstle
Copy link
Collaborator Author

@hemajv @oindrillac @cdolfi responded to PR

Copy link
Contributor

@cdolfi cdolfi left a comment

Choose a reason for hiding this comment

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

lgtm! Good to merge once oindrilla and hema take a look

Copy link
Member

@oindrillac oindrillac left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @JamesKunstle

@sgoggins
Copy link

@JamesKunstle : I'm running your fork and get an error that this notebook does not exist: Whole-Game-Arg-Feature-Processing.ipynb

@JamesKunstle
Copy link
Collaborator Author

@sgoggins uhhhh weird. I'll look into that.

Copy link
Contributor

@hemajv hemajv left a comment

Choose a reason for hiding this comment

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

thanks for addressing the comments @JamesKunstle!
/lgtm 👍

@JamesKunstle
Copy link
Collaborator Author

@sgoggins I definitely don't get that error. Are you getting it when you boot this notebook and try to run it?

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

Successfully merging this pull request may close these issues.

6 participants