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

Feat/slicing kedro viz #1872

Closed
wants to merge 10 commits into from
Closed

Feat/slicing kedro viz #1872

wants to merge 10 commits into from

Conversation

rashidakanchwala
Copy link
Contributor

@rashidakanchwala rashidakanchwala commented Apr 23, 2024

Description

Experimentation on filtering pipelines by slicing them the way kedro does.

kedro-viz-slice

Development notes

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Just had a first look. I don't see anything major I would change. It's looking good! Can still get on a call to have you explain things in more detail though. I'm sure I'm missing something.

Comment on lines +14 to +22
const getEdges = createSelector(
[getEdgeIDs, getEdgeSources, getEdgeTargets],
(edgeIDs, edgeSources, edgeTargets) =>
edgeIDs.map((id) => ({
id,
source: edgeSources[id],
target: edgeTargets[id],
}))
);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason this logic can't live within the getEdgesByNode selector? It doesn't look like you're using it anywhere else.

* @returns {Object} An object containing edges mapped by source and target nodes.
*/

export const getEdgesByNode = createSelector([getEdges], (edges) => {
Copy link
Member

Choose a reason for hiding this comment

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

Don't think you need to export this, do you?

Comment on lines 102 to 104
filteredNodeIDs = linkedNodesBetween;

return filteredNodeIDs;
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to reassign it here?

Suggested change
filteredNodeIDs = linkedNodesBetween;
return filteredNodeIDs;
return linkedNodesBetween;

@rashidakanchwala
Copy link
Contributor Author

I am closing this PR but leaving the branch for @Huongg to work from it.

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.

3 participants