Improvements in linking #1189
Improvements in linking #1189
Conversation
astrofrog
force-pushed
the
improved-linking
branch
from
cd5a4f9 to
f0c2efc
Compare
There was a problem hiding this comment.
The UI improvement here is great, so big :+1 from me on that.
As you know I've been burned by both merged and un-merged components in the past, which makes me a little nervous about hidden gotchas associated with switching again. However I trust your current thinking better than my fuzzy memory at this point. I know the main painpoint with unmerged ComponentIDs was duplication in the UI, and you have a solution for that here.
You may also want to double check that if you stitch together, say, 5 datasets with the same component IDs, that there isn't any weird or slow behavior with how data overlays behave (i.e. something getting stuck in a component ID loop).
I don't have any problems with the parent ComponentIDs. Looks good to me!
| @@ -338,6 +338,10 @@ def __setgluestate__(cls, rec, context): | |||
| return cls(None, rec['axis'], rec['world']) | |||
|
|
|||
|
|
|||
| class AutoDerivedComponent(DerivedComponent): | |||
There was a problem hiding this comment.
small nit: I'm not sure if you want the name of this to be tied to its origin (added by machine) or its functional organization (should be invisible in certain contexts)
| else: | ||
| result = "%s <-> %s" % (self._to, self._from) | ||
| if self._inverse is None: | ||
| result = "%s ← %s(%s)" % (self._to.to_html(), self._using.__name__, args) |
There was a problem hiding this comment.
small nit -- does unicode work here? Or are these HTML codes required?
There was a problem hiding this comment.
I can't get quite the same result with unicode, so leaving this.
…onents that only exist because links have been set up - we can then use this to avoid showing all automatically added components in component lists (e.g. in the link editor)
…ed that these were ignored)
…ivedComponent when adding components to data
…splay the original data when using the link editor, so as to know which component comes from which dataset
…imary components, the component ID is returned (even if one or more are present with matching labels in the derived components).
astrofrog
force-pushed
the
improved-linking
branch
from
f0c2efc to
92f324d
Compare
…ntID.hidden settable, and hide automatically generated derived components rather than requiring a new AutoDerivedComponent class
About
This re-thinks some aspects of data linking based on feedback from users. Some things that have confused users in the past are:
Due to the de-duplication of component IDs, if one links e.g. component
AandC, the link originally appeared asA <-> C, but when opening the link editor later, the link was changed toA <-> Amaking it difficult to know what is what.Following linking, some linked components appeared in the list of components of the link editor window. This would cause e.g. several instances of
Pixel Axis 0 [z]to appear in the list, making it impossible to know which was which.In the list of links, when linking e.g. 5 datasets all with components
RAandDec, one would end up with many links of the formRA <-> RAwith no indication of which data were linked. In principle, component IDs don't have to be attached to a dataset, but in 99% of use cases it will be.This PR attempts to change this.
Removing the merging of component IDs
First, this PR gets rid of the de-duplication of component IDs. This de-deduplication was discussed extensively in #508 and there are some good reasons to do it, but unfortunately I think the cons outweigh the pros. Namely, the issue with de-duplication is issue 1 above (the original names of components is lost) and also that it's then impossible to un-glue all the links and return the data to the original state. Telling people that they've messed up the links and need to restart glue is not ideal.
Introducing AutoDerivedComponent
A consequence of the removal of merging is that the link editor started showing all the linked components for each dataset. This PR introduces a trivial sub-class of
DerivedComponentcalledAutoDerivedComponentwhich is used for derived components added via links (to differentiate it from user-created derived components). This then allows us to filter out theAutoDerivedComponentsin lists of components, since these should never really be seen by the user. This addresses problem 2 above.Assigning parent data objects to component IDs
This may be a little controversial, but is needed to address problem 3. In 99% of use cases, component IDs are specific to a given dataset - that is, without the de-duplication, it's likely extremely rare that users set up data objects which share a component ID. In this PR, component IDs have a
parentattribute that is the data they were first attached to. If a component ID is then added to a new dataset, the parent will still refer to the original dataset the component ID was added to. On the plus side, this then allows us to show this in the list of links:Note that this uses rich text, so I've added a
to_htmlmethod on ComponentIDs and on ComponentLinks. ComponentIDs without parent data are still allowed and can be used in ComponentLink with arithmetic expressions.Re-layout of the link editor
I've done some re-layout of the link editor to move the functions/gluing options in between the two columns of components and moved the list of links to the bottom (this ensures that even if the lines with the links are long, they are still readable). I've also made sure that horizontal scroll bars appear if needed in the list of links.
Simple mode
Advanced mode
@ChrisBeaumont @stscieisenhamer - it would be great if you could take a look at this given the discussion in #508 - thanks!