-
Notifications
You must be signed in to change notification settings - Fork 29
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(api): publish traversed spec to gosling api #978
Conversation
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.
Thank you for making this change!
One comment: Could we make this a 'standard' Gosling API function? This will require a small change, which are
- Adding
specTraversed
as an API event type:gosling.js/src/gosling-schema/gosling.schema.ts
Lines 370 to 380 in 71a2a28
export type _EventMap = { mouseOver: PointMouseEventData; click: PointMouseEventData; rangeSelect: RangeMouseEventData; rawData: CommonEventData; trackMouseOver: TrackApiData; trackClick: TrackApiData; // TODO (Jul-25-2022): with https://github.com/higlass/higlass/pull/1098, we can support circular layouts onNewTrack: OnNewTrackEventData; onNewView: OnNewViewEventData; location: LocationEventData; }; - Replace the code to publish the event, like how we do for other API events, e.g.,
gosling.js/src/tracks/gosling-track/gosling-track.ts
Lines 478 to 481 in 71a2a28
publish('location', { id: context.viewUid, genomicRange: genomicRange });
This way, users can use the gosRef.current.api.subscribe('specTraversed', (...args) ⇒ console.log(args));
pattern.
src/compiler/compile.ts
Outdated
if (PubSub) { | ||
PubSub.publish('specTraversed', { | ||
id: specCopy.id, | ||
data: specCopy |
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 more intuitive and consistent with how we use "data" and "spec" in other API functions
data: specCopy | |
spec: specCopy |
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.
Ah I see, I had already written this piece before the standardization! Thanks!
src/compiler/compile.ts
Outdated
// used for alt-gosling | ||
try { | ||
if (PubSub) { | ||
PubSub.publish('specTraversed', { |
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.
Regarding the name of the event, how about specProcessed
instead? I think users might not understand what the traverse means. And, we can explain what is the different between the input spec and the processed spec briefly in the documentation.
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.
That sounds like a good idea! I indeed was fidgetting around with a good name, specProcessed is more intuitive
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.
Sorry, bikeshedding, maybe specResolved
?
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 like specResolved
, which I think gives a better impression that the spec is not converted into another format, but rather the spec is slightly modified to resolve things!
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.
Wouldn't resolved give the idea that the spec is fully changed into the 'final form' that it gets for gosling? Because that isn't the case here, it is just partially processed. I'm not sure what the convention is, but I'm afraid resolved gives off the wrong 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.
How has the input been modified at this point in the program?
The input spec is modified to have default values filled in, and some of the users' incorrectly-specified properties are fixed. Some parts of the spec are not touched by the program (e.g., when values are already specified by users, so no need to add default values), which I think is what @thomcsmits was referring to by "it is just partially processed"
.
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.
Hm, we don't emit a fully processed spec ("fully processed" as in the final state that it becomes in Gosling), but that might happen in the future
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 don't have truely strong opinions on this, so I'm happy to switch it to specResolved still
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.
#985 (as this PR is already merged)
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 don't really have a strong opinion about "resolved" vs "processed", but think this could be a good exercise to be explicit about the state of the gosling spec when it is emitted. "Resolved" and "processed" convey that completion of some transformations/resolution, which is not the case. I have left a longer comment in #985
add to eventmap
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.
Thanks for making updates! They looks good to me.
Fix #
Toward #
Change List
Checklist