-
Notifications
You must be signed in to change notification settings - Fork 0
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
Edge slicing #372
Edge slicing #372
Conversation
✔️ Deploy Preview for multimatrix ready! 🔨 Explore the source changes: 63f104e 🔍 Inspect the deploy log: https://app.netlify.com/sites/multimatrix/deploys/6229007b9bf10b00083fce43 😎 Browse the preview: https://deploy-preview-372--multimatrix.netlify.app/ |
@JackWilb can you take a look at the to-do I listed? I think the reason it doesn't work is because of when the edge variable list is created. It might need to be generated |
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.
Looks good so far, the UI needs some work, as you said. I think it's good to go, but maybe we should hold off until you have a PR for the UI changes.
Also, is there anything different in the edge slicing, and edge slices component that I haven't review in the multidyno?
src/components/EdgeSlicing.vue
Outdated
return cleanedVariables; | ||
} | ||
|
||
const cleanedEdgeVariables = computed(() => { |
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.
Can these be moved to the store, we have similar logic for the node variable lists there. It would make sense to put similar code together, and maybe we can even reduce some of the logic needed
It seems that the list generation relies on |
nope! I just refactored some of the code so that it works with the MultiMatrix setup, but nothing new yet |
Made some bigger changes:
Left to do: I need some help with the type errors (sorry, I just can't figure out what's going on this time around) |
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'm planning to give you a couple rounds of review. The first here will be higher level issues, and then I'll comment on specific code that might be updated in the next round.
Here are a few other things I noticed:
- I can't see a tooltip
- It was unintuitive to me that some of the slices were orange and some were not when I first made a network. The tooltip will help with this, but is there some other way we can show what the orange represents?
- There isn't enough padding on the "Edge Slices" at the top of the vis
- It looks like the SVG you added could be smaller from some playing around. I think 30px high would make it fit in a bit more. Unless I missed something that goes below each of the bars.
- I found it unintuitive that there were separate variables for start and end. Looking at your screenshot, it seems a bit more clear, but why would we want it across 2 variables? Is that something we could hide unless the user asks for it, knowing that
end variable
is a separate column in their data? - There should be a button somewhere that will clean all the edge slicing away. It seems that the best way to do that currently is to remove the variables from the panel and re-run the slicing, but that's too much work for a user. If it's one button to clear, that will be much nicer.
- We should open an issue to make sure that the sorting we implement is able to take information from slices instead of the whole network
- Is there a nice way we can show what ranges of values fall into each time slice?
src/components/ConnectivityQuery.vue
Outdated
@@ -228,6 +228,8 @@ export default defineComponent({ | |||
store.dispatch.updateNetwork({ network: newNetwork }); | |||
loading.value = false; | |||
store.commit.setDirectionalEdges(true); | |||
// remove sliced network | |||
store.commit.setSlicedNetwork([]); |
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.
Maybe this should just be a part of updateNetwork
? It seems that every time it updates, we'd want to unset this
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.
Great idea!
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.
Looking good so far! I have a smaller round of changes here that will just help with some papercuts
} | ||
}); | ||
|
||
function sliceNetwork() { |
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.
This fails silently if the range specified by user is outside of the values of the data. For example, I have data from 0 to 31 and if I specify -100 to 31, it fails without alerting the user. We could have some simple validation and show errors to the user when it happens
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.
resolved for both number inputs and date inputs
const selectedArray: ComputedRef<boolean[]> = computed(() => Array.from(Array(timeRangesLength.value), (_, x: number) => (x === 0))); | ||
function isSelected(key: number) { | ||
selectedArray.value.fill(false); | ||
selectedArray.value[key] = true; |
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.
Changing a value on a computed ref like this is dangerous. You should change the computedref to a ref and a watcher on timeRangesLength
just to reset it when it changes, and then you can modify the array without it magically getting updated by the computed component
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.
FWIW (might not apply here) you can use a "computed setter" (V2: https://v2.vuejs.org/v2/guide/computed.html#Computed-Setter) or a "writable computed" (V3: https://vuejs.org/guide/essentials/computed.html#writable-computed) to give semantics to "changing a value on a computed ref".
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.
Ok, this has also been fixed!
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.
Noticed a couple of small things, but in general, it's looking good to go!
src/components/EdgeSlicing.vue
Outdated
@@ -490,7 +491,7 @@ export default defineComponent({ | |||
color="primary" | |||
block | |||
depressed | |||
:disabled="!isValidRange && !isDate" | |||
:disabled="!isValidRange && !isDate && isNumeric" |
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.
Is this right? Shouldn't it be just !isValidRange
? Why would we disable it if it's numeric?
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.
This is so that the isValidRange
only does the check for isNumeric
selections. Otherwise, you can't slice along categories
Does this PR close any open issues?
No.
Give a longer description of what this PR addresses and why it's needed
This is incorporating the functionality of MultiDynamic into MultiMatrix. It will be more generalized so that it can also account for categorical edge types.
Provide pictures/videos of the behavior before and after these changes (optional)
Edge slicing lets you pick numerical and date-formated edge variables and create slices.
Once the slices are created, you can navigate the network along the slice
Are there any additional TODOs before this PR is ready to go?
TODOs: