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

Bipartite network api update #765

Merged
merged 4 commits into from
Jan 16, 2024
Merged

Bipartite network api update #765

merged 4 commits into from
Jan 16, 2024

Conversation

d-callan
Copy link
Contributor

resolves #764

this will keep from breaking things that are currently working. idk if there were other changes you were anticipating..

@d-callan d-callan requested a review from asizemore January 10, 2024 18:02
@d-callan
Copy link
Contributor Author

side note.. seems the bipartitenetwork viz is maybe more properly called a correlationbipartitenetwork ?? not urgent obviously, just a thought i had while looking at this

Copy link
Member

@asizemore asizemore left a comment

Choose a reason for hiding this comment

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

I think it's important we separate the data we use to make the network from the drawing of the network. This concept will become more crucial as we get more data for nodes and links.
Currently there is a BipartiteNetworkData type defined in both the components and eda libraries. They are currently kept in sync (there's a long discussion about this in another pr if you're interested). But the point is that I think we should un-sync them. The bipartite data coming from the backend should remain as BipartiteNetworkData with the api changes, and then the components type should be the actual network, not the data. So it should say "strokeWidth" instead of weight, for example. I want to call that type BipartiteNetwork but that could be confusing with the component being named the same thing. @dmfalke might have an opinion here.

At the very least, if we do not decide to go the above route, i think there are some more strokeWidth instances in the components library that should be updated. For example the Link component. I would guess when you tested the changes all the links were drawn with the same stroke width?

@d-callan
Copy link
Contributor Author

I'd be curious to read through the other discussion. And I have questions about the stroke vs weight thing for the component, but I'll save them for Mon probably

@d-callan
Copy link
Contributor Author

ok. so i changed some names to separate the viz and component types as requested. i think given how things are, its reasonable. though i do wonder if were potentially backing ourselves into a corner. are we very confident the component will never need to know about the underlying data? that will always be either the backend or the vizs job? given the way weve engineered things so far im ok w continuing w that assumption, just made me a bit nervous.

@d-callan d-callan requested a review from asizemore January 16, 2024 16:14
@asizemore
Copy link
Member

Yes i think from a philosophical standpoint the component should not know about the data. It should be only drawing a network, exactly as given. It's everyone else's job to provide the context, and let the component be totally independent. There might be something that breaks that idea someday, but for now this setup seems like it should best handle all of our use cases.

@d-callan d-callan merged commit 5c0c497 into main Jan 16, 2024
1 check passed
@d-callan d-callan deleted the bipartite-network-api-update branch January 16, 2024 16:36
@dmfalke
Copy link
Member

dmfalke commented Jan 16, 2024

I want to call that type BipartiteNetwork but that could be confusing with the component being named the same thing. @dmfalke might have an opinion here.

I'm not sure which type you want to call BipartiteNetwork. Regardless, my instinct is to say that we should not have a component named BipartiteNetwork, and instead should be something like BipartiteNetworkPlot. My thinking is that the component represents one way to "visualize" the network data. Does that make sense?

@asizemore
Copy link
Member

@dmfalke sounds good. I added this renaming task to the bipartite network polishing gh issue.

@dmfalke
Copy link
Member

dmfalke commented Jan 16, 2024

Great. I wonder if there are other components that should have Plot appended to their name. Would you mind adding a note to check in the polish-up issue?

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.

Correlation: update api
3 participants