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

Degree filter on large query #416

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Degree filter on large query #416

wants to merge 7 commits into from

Conversation

gotdairyya
Copy link
Contributor

@gotdairyya gotdairyya commented Jul 23, 2022

Does this PR close any open issues?

Closes #415

Give a longer description of what this PR addresses and why it's needed

At the moment we don't have any way to display results from queries that are > 100.
Adding a check to see how large the query result is will trigger a filtered matrix.

Provide pictures/videos of the behavior before and after these changes (optional)

Are there any additional TODOs before this PR is ready to go?

TODOs:

  • Find a semantically meaningful min degree value
  • Change the hover text for filter to show # of children

@netlify
Copy link

netlify bot commented Jul 23, 2022

Deploy Preview for multimatrix ready!

Name Link
🔨 Latest commit ba9a959
🔍 Latest deploy log https://app.netlify.com/sites/multimatrix/deploys/62e423e3bbf92e0008d53eda
😎 Deploy Preview https://deploy-preview-416--multimatrix.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@gotdairyya
Copy link
Contributor Author

Netlify does not like how I am treating the hover object -- I tried to add a check for it but it didn't work. Line 167, MultiMatrix.vue

@gotdairyya
Copy link
Contributor Author

Test Query: RPC1,
Node 0 Label ~= CBB

@gotdairyya gotdairyya marked this pull request as ready for review July 25, 2022 22:01
@JackWilb JackWilb self-requested a review July 26, 2022 15:21
Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

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

There are some questions that I have before an approval. Thanks for this feature

@@ -389,6 +390,10 @@ const {
state.nodeDegreeDict = degreeObject.nodeDegreeDict;
},

setMinDegree(state, minDegree: number) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't have a setMaxDegree, why can't we just get this info from setDegreeNetwork or another setDegreeEntries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to keep it separate because in ControlPanel.vue minDegree is a computed value and it should be 0 unless it is a large query

Copy link
Member

Choose a reason for hiding this comment

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

We met to discuss this. There are likely some edge cases that this will break

src/components/ConnectivityQuery.vue Outdated Show resolved Hide resolved
src/components/ConnectivityQuery.vue Outdated Show resolved Hide resolved
@gotdairyya
Copy link
Contributor Author

gotdairyya commented Jul 27, 2022

Created issue #417 because although this solves larger query results, it is not a solution for very large query returns that crash the browser.

Example query:
workspace: marclab
network: RC1---
query: node0.Label ~= CBB

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.

Visualize large query results
2 participants