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

Add the ability to create one-to-one 'join_on_key'-type links to the GUI link editor #2313

Merged
merged 14 commits into from
Oct 11, 2022

Conversation

jfoster17
Copy link
Member

@jfoster17 jfoster17 commented Aug 3, 2022

Add join_on_key to UI

Description

This is a combination of a plug-in and core changes to the link_editor to enable the creation, display, and removal of join_on_key type links (managed through a Join_Link object). This address the immediate need in #2215.

@jfoster17 jfoster17 marked this pull request as draft August 3, 2022 19:53
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #2313 (44bf242) into main (f8c1e96) will increase coverage by 0.02%.
The diff coverage is 90.32%.

❗ Current head 44bf242 differs from pull request most recent head 1b0fb5b. Consider uploading reports for the commit 1b0fb5b to get more accurate results

@@            Coverage Diff             @@
##             main    #2313      +/-   ##
==========================================
+ Coverage   88.06%   88.08%   +0.02%     
==========================================
  Files         247      247              
  Lines       23476    23527      +51     
==========================================
+ Hits        20673    20723      +50     
- Misses       2803     2804       +1     
Impacted Files Coverage Δ
glue/dialogs/link_editor/state.py 94.44% <66.66%> (-1.23%) ⬇️
glue/dialogs/link_editor/qt/data_graph.py 92.32% <81.81%> (-0.23%) ⬇️
glue/core/link_helpers.py 85.88% <96.42%> (+1.27%) ⬆️
glue/core/link_manager.py 97.77% <100.00%> (+0.13%) ⬆️
glue/conftest.py 67.85% <0.00%> (+4.76%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jfoster17 jfoster17 changed the title WIP: Add join on key to UI Add join on key to UI Aug 5, 2022
@jfoster17
Copy link
Member Author

Hey @astrofrog -- can you take a look at this solution? It's not... ideal, but it basically works. Basically I have a Join_Link(LinkCollection) object that represents the join_on_key operation and then the link_manager knows how to use this object to add or remove the appropriate _key_joins. Here are some of my concerns.

  1. Because the key joins are not the same thing at the Join_Link object, if users (or code) directly calls join_on_key, this won't create a Join_Link and so there will be no representation of the join in the UI.
  2. Because Join_Links are funny kinds of things, I end up with data_collection._link_manager_external_links holding Join_Links, but Join_Links do NOT show up in data_collection.links
  3. Currently I have this limited to "Joining on single components". (I don't see a fundamental reason I couldn't extend it to multiple components or one-to-many joins I just haven't done that yet).
  4. The display in the link manager is a little funny -- I draw these Join_Links as dashed lines, but a new Join_Link does not show up as dashed right away, only after closes and re-opening the link manager. This can probably be fixed in link_editor.qt.data_graph but I haven't dug into it a lot (it's not obvious what to display if two datasets are both joined_on_key and glued through a traditional link for some reason).
  5. I set up Join_Link as a plugin originally, hoping that I might be able to do things all as a plug-in. Now I have core components assume this plugin exists, but is obviously not ideal (a user might disable the plug-in). Presumably I should move it to core.link_helpers?

@jfoster17 jfoster17 marked this pull request as ready for review August 5, 2022 19:39
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

This is looking good so far! A few comments below.

Because the key joins are not the same thing at the Join_Link object, if users (or code) directly calls join_on_key, this won't create a Join_Link and so there will be no representation of the join in the UI.

To some extent, join_on_key should perhaps become deprecated API once we are confident it can be treated as a regular link - that is, in future maybe creating a JoinLink should be the preferred way to do it. So I wouldn't worry about this.

Because Join_Links are funny kinds of things, I end up with data_collection._link_manager_external_links holding Join_Links, but Join_Links do NOT show up in data_collection.links

Ok I don't know immediately why that would be the case, but we should be able to get that to work in future.

Currently I have this limited to "Joining on single components". (I don't see a fundamental reason I couldn't extend it to multiple components or one-to-many joins I just haven't done that yet).

Sounds good

The display in the link manager is a little funny -- I draw these Join_Links as dashed lines, but a new Join_Link does not show up as dashed right away, only after closes and re-opening the link manager. This can probably be fixed in link_editor.qt.data_graph but I haven't dug into it a lot (it's not obvious what to display if two datasets are both joined_on_key and glued through a traditional link for some reason).

Ok yes we can look into this later

I set up Join_Link as a plugin originally, hoping that I might be able to do things all as a plug-in. Now I have core components assume this plugin exists, but is obviously not ideal (a user might disable the plug-in). Presumably I should move it to core.link_helpers?

Yes I think maybe it should just be a core link type.

The bottom line is that I'm happy for this to go into the core package, and I think it was overdue that joins become normal links - thanks @jfoster17!

This will also need a changelog entry.

glue/core/link_manager.py Outdated Show resolved Hide resolved
glue/dialogs/link_editor/qt/data_graph.py Outdated Show resolved Hide resolved
glue/plugins/join_on_key/link_helpers.py Outdated Show resolved Hide resolved
glue/plugins/join_on_key/link_helpers.py Outdated Show resolved Hide resolved
@jfoster17
Copy link
Member Author

I assume you would like to be rebase and add a changelog entry when this is good-to-go?

@astrofrog
Copy link
Member

Yes that would be great, thanks!

@astrofrog astrofrog changed the title Add join on key to UI Add the ability to create one-to-one 'join_on_key'-type links to the GUI link editor Oct 11, 2022
@astrofrog astrofrog merged commit efca117 into glue-viz:main Oct 11, 2022
@jfoster17 jfoster17 deleted the add-join-on-key-to-ui branch March 21, 2023 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants