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.

Sign up for GitHub

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

Jump to bottom

Better connect API #5

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Better connect API #5

wants to merge 3 commits into from

Conversation

akx
Copy link
Member

@akx akx commented Jul 8, 2021 •
edited
Loading

This PR adds more functions for creating edges.

For creating parameter edges, you currently need to

EdgeDescriptor(node=t1, type="parameter", key="learning_rate").to(EdgeDescriptor(node=t2, type="parameter", key="learning_rate"))

With this PR, the equivalent syntax is

t1.parameter_edge("learning_rate").to(t2.parameter_edge("learning_rate"))

Much nicer.

❓ However, *_edge is a bit of a misnomer, since what it really returns is an edge descriptor you can connect to... Is that a problem?

@akx akx added the type:enhancement New feature or request label Jul 8, 2021
@akx akx requested review from JuhaKiili, a team and tokkoro and removed request for a team July 8, 2021 11:40
@akx akx force-pushed the better-connect-api branch 2 times, most recently from 49a3de2 to 5e223bc Compare July 8, 2021 11:53
@akx akx mentioned this pull request Jul 8, 2021
Merged
@akx akx force-pushed the better-connect-api branch from 5e223bc to d07773f Compare July 8, 2021 12:58
@akx akx changed the title Better connect API + tests Better connect API Jul 8, 2021
tokkoro
tokkoro approved these changes Jul 9, 2021
View reviewed changes
Copy link

@tokkoro tokkoro left a comment

Choose a reason for hiding this comment

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

Code looks clean

@tokkoro
Copy link

tokkoro commented Jul 9, 2021

Could @JuhaKiili or someone else who is more in touch with the customer to comment on the *_edge and its misnomerity?

@akx akx marked this pull request as draft August 11, 2021 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tokkoro tokkoro tokkoro approved these changes

@JuhaKiili JuhaKiili Awaiting requested review from JuhaKiili

Assignees
No one assigned
Labels
type:enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akx @tokkoro