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!: merge axis channels (e.g., x and xe); group channels as an encoding property #575

Closed
wants to merge 14 commits into from

Conversation

sehilyi
Copy link
Member

@sehilyi sehilyi commented Nov 1, 2021

Fix #506
Fix #482

Summary of Changes

In addition to #533, this PR changes grammar around visual channels.

This PR contains a relatively large amount of changes, but many parts are changes on the examples (src/example/*.ts) and tests (*.test.ts) reflecting the grammatical changes that have been introduced in this PR. This PR introduces the following major grammatical changes.

C1. Axis channels are combined

from:

x: { field: 'start', type: 'genomic', axis: 'top', linkingId: 'overview' },
xe: { field: 'end', type: 'genomic', axis: 'top' }

to:

x: { startField: 'start', endField: 'end', type: 'genomic', axis: 'top', linkingId: 'overview' }

This way, axis channels convey a more clear meaning:

  • multiple axes do not exist for the x-axis (i.e., x and xe) or the y-axis (i.e., y and ye).
  • Instead, multiple fields can be used for a single axis (i.e., startField and endField in an x channel).

More importantly, this prevents users from making uncertain specs, such as

  • using different types for the single axis (e.g., nominal field for y and then quantitative field for ye)
  • defining properties multiple times for a single axis that should be defined only once (e.g., axis: 'top' or linkingId: 'overview'):
x: { field: 'start', type: 'genomic', axis: 'top', linkingId: 'overview' },
xe: { field: 'end', type: 'genomic', axis: 'bottom', linkingId: 'ab' }

C2. The encoding property is added

Track now has a encoding property that wraps all channels:

tracks: [{
   mark: 'line',
   encoding: {
      x: { startField: 'start', endField: 'end', type: 'genomic', axis: 'top', linkingId: 'overview' },
      color: { value: 'black' }
   },
   width: 100,
   height: 30
}]

This is consistent with the Vega-Lite and is beneficial in gos (gosling-lang/gos#34 (comment)).

C3. TemplateTrack modified accordingly

As we made a grammatical change to channels (i.e., x and xe --> x: { startField: ...., endField: ..., ...}), the template mapper logic was changed as well:

// part of a TemplateTrackDef spec
x: { base: { startField: 'startPosition', endField: 'endPosition', ... }

C4. DataTrack is removed entirely

We did not support DataTrack anymore, but it was still included in our code. I removed all related codes. We could support a similar feature with Track Templates in Gosling in the future:

{ template: 'vector-track', data: { ... }, width: 1000, height: 30}

To Test Locally

Run the following command to update your local gosling.schema.json since this PR updates the grammar.

yarn schema

BREAKING CHANGE

This PR introduces breaking changes. To change your existing specification for previous versions (~v0.9.9), you need to change your spec following the instructions described below.

If you are using multiple fields for a single axis in your spec:

x: { field: 'start', type: 'genomic', axis: 'top', linkingId: 'overview' },
xe: { field: 'end', type: 'genomic', axis: 'top' }

You need to combine them to a single axis using up to four fields (i.e., startField, endField, startField2, endField2):

x: { startField: 'start', endField: 'end', type: 'genomic', axis: 'top', linkingId: 'overview' }

Single-field axes are not changed (i.e., field properties are used):

x: { field: 'position', type: 'genomic', axis: 'top', linkingId: 'overview' }

Add an encoding property to all tracks.

From:

tracks: [{
   data: ...,
   mark: ...,
   x: ...,
   y: ...,
   ...
}]

To:

tracks: [{
   data: ...,
   mark: ...,
   encoding: {
      x: ...,
      y: ...,
      ...
   },
   ...
}]

TODO

  • Update all example specs that use multiple fields for a single axis
  • Update encoding parts of all marks
  • Update all tests
  • Rename '|xe - x|' threshold to 'assignedWidth'?

@sehilyi sehilyi changed the title feat!: merging and grouping encodings feat!: merging axis channels; explicit encoding property Nov 1, 2021
@sehilyi sehilyi changed the title feat!: merging axis channels; explicit encoding property feat!: merge axis channels; group channels as a encoding property Nov 3, 2021
@sehilyi sehilyi changed the title feat!: merge axis channels; group channels as a encoding property feat!: merge axis channels (e.g., x and xe); group channels as a encoding property Nov 3, 2021
@sehilyi sehilyi changed the title feat!: merge axis channels (e.g., x and xe); group channels as a encoding property feat!: merge axis channels (e.g., x and xe); group channels as an encoding property Nov 3, 2021
@@ -2,7 +2,7 @@ export { name, version } from '../package.json';
import GoslingSchema from '../schema/gosling.schema.json';
import ThemeSchema from '../schema/theme.schema.json';

export type { GoslingSpec, TemplateTrackDef } from './core/gosling.schema';
export type { GoslingSpec, Encoding, TemplateTrackDef } from './core/gosling.schema';
Copy link
Member Author

@sehilyi sehilyi Nov 3, 2021

Choose a reason for hiding this comment

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

Would need to ensure to have Encoding in the schema.json file.

Reference: #565 (comment)

@@ -15,9 +15,6 @@ export function compile(
// Make sure to keep the original spec as is
const _spec = JSON.parse(JSON.stringify(spec));

// Override default visual encoding (i.e., `DataTrack` => `BasicSingleTrack`)
overrideDataTemplates(_spec);
Copy link
Member Author

@sehilyi sehilyi Nov 3, 2021

Choose a reason for hiding this comment

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

Removed this since DataTrack had been deprecated.

@@ -459,9 +436,11 @@ export interface ChannelDeepCommon {
field?: string;
}

export interface X extends ChannelDeepCommon {
Copy link
Member Author

Choose a reason for hiding this comment

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

Key changes can be found in this file.

@sehilyi
Copy link
Member Author

sehilyi commented Nov 16, 2021

@manzt, sorry that this update was delayed. It turns out to be more significant updates than I expected.

Can you review this PR? I would be interested in hearing from you whether the grammatical changes look okay and these updates work for you to continue working on gosling-lang/gos#71. Please review code bases as deeply as you would like to.

@manzt
Copy link
Member

manzt commented Nov 16, 2021

No problem! Won't be able to look today but probably tomorrow.

@manzt manzt self-assigned this Dec 10, 2021
@manzt
Copy link
Member

manzt commented Dec 10, 2021

@sehilyi I completely lost track of this. Will review this weekend -- sorry! Do you have any thoughts regarding how to reserve these merge conflict?

@sehilyi
Copy link
Member Author

sehilyi commented Dec 11, 2021

@manzt No worries! I was basically thinking of manually resolving conflicts which I hope to be manageable.

Also, please let me know if all grammatical changes introduced in this PR make sense to you. Since this PR introduces breaking changes, I would like to merge this carefully.

@sehilyi
Copy link
Member Author

sehilyi commented Jul 5, 2022

@manzt Since this will introduce breaking changes and is too far behind the current version, I am reluctant to resolve conflicts and merge this.

I think we can revisit this (at least making an encoding property in a SingleTrack) later when we plan to make a major version bump.

@sehilyi sehilyi closed this Jul 5, 2022
@manzt
Copy link
Member

manzt commented Jul 5, 2022

Agreed!

@manzt
Copy link
Member

manzt commented Jul 6, 2022

@sehilyi I just wanted to apologize that your hard work here never got a thorough review. I think this PR had several different aims and touched many internals of Gosling that at the time I struggled to understand.

@sehilyi
Copy link
Member Author

sehilyi commented Jul 7, 2022

@manzt No worries. This was such a huge PR. And I am sure we can revisit this and repurpose it for future PRs.

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.

More precise encoding types Group channels as encoding
2 participants