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

Multi scale axis (-- extends right-y-axis) #209

Closed
wants to merge 22 commits into from

Conversation

wasbridge
Copy link
Contributor

This fork is a branch which is on top of the right-y-axis branch. This means it adds everything that the right-y-axis branch does PLUS a new axis type (multiscaling linear y axis).

This new axis is used to represent multiple scales on the same axis. Both this and the right-y-axis feature are coded in such a way as to not affect any existing deployments unless they choose to use the new features.

Jaime -- if you would like I would be happy to walk you through these changes, how to configure the new chart types, and what the new chart types look like (and do)

@wasbridge wasbridge changed the title Multi scale axis Multi scale axis (-- extends right-y-axis) Mar 11, 2015
this.options needs to be passed in because the sizing info most probably have changed since the axis was created
@jaimedp
Copy link
Contributor

jaimedp commented Mar 12, 2015

Give me some time to review all the changes and then maybe we can walk through all of it. I'm a bit concerned that now all visualizations need to be aware of the left/right axis difference and the amount of code that is now dependent on knowing the difference between left/right axes. I always thought we could do it by configuring a new axis and abstracting how/what the visualizations use, so this.yScale returns the correct axis for the visualization. Maybe even something like:

new Contour({...})
  .cartesian()
  .line(data)
  .rightAxis()
  .line(dataForRightAxis)
...

but maybe this is not possible with the current structure.

@wasbridge
Copy link
Contributor Author

Definitely review, take your time -- there are many changes.

To clarify -- what the visualizations really need to know is what is the
proper scaling function. In the end the visualizations care nothing for
the axis -- the question they need to answer is 'What scale function should
I apply to this scalar value Y from this series S?' You'll see this when
you look into how the multi-scale-linear axis works.

Your code proposal could potentially work for the distinction between y
axes, but wouldn't work for multiple scales on the same axis.

On Thu, Mar 12, 2015 at 12:53 PM, Jaime del Palacio <
notifications@github.com> wrote:

Give me some time to review all the changes and then maybe we can walk
through all of it. I'm a bit concerned that now all visualizations need to
be aware of the left/right axis difference and the amount of code that is
now dependent on knowing the difference between left/right axes. I always
thought we could do it by configuring a new axis and abstracting how/what
the visualizations use, so this.yScale returns the correct axis for the
visualization. Maybe even something like:

new Contour({...})
.cartesian()
.line(data)
.rightAxis()
.line(dataForRightAxis)
...

but maybe this is not possible with the current structure.


Reply to this email directly or view it on GitHub
#209 (comment).

@wasbridge
Copy link
Contributor Author

Hey -- just checking in -- noticed you committed a bunch of changes to contour/master and now this doesn't cleanly merge.

If I were to make this branch cleanly merge again would you accept it?

If not is it the feature you don't want (right y axis, and multi-scale y axis)? Or the implementation? If it is the feature unfortunately I require both and can't see how I could do it without modifying contour, but if its the implementation I am glad to hear how you would like to see it structured.

I would like to get this merged upstream sooner rather then later. Since it is such a wide change the older it gets the harder it gets to merge.

@jaimedp
Copy link
Contributor

jaimedp commented Mar 16, 2015

Hey, so far my thinking is that the multiple axes is not something I specially want to support (bad practice and all that :), but I want to make it easy for you to have a specific extension that allows for that. My concern with the current implementation is that the concept of the multiple axes get's leaked into every part of cartesian() and the visualizations. So give me a few days to come up with a solution that does not involve so much dependencies in the axes and I'll let you know. How does that sounds?

OTHO, I made a few changes to master that relate to jshint and code standards, and also fixed some tests that were broken.

@wasbridge
Copy link
Contributor Author

Honestly I am a bit dubious about how we could achieve your desire to
cleanse cartesian and the visualizations of the notions of multiple scales
while still supporting multiple scales and multiple axes.

With that said I understand not wanting to support multiple axes/scales
because it is such bad practice, but I don't have that choice I have to
support this stuff for better or worse.

Starting with your pseudo code which you attached above. It could
theoretically work for leftYAxis/rightYAxis but I can't see how it could
work with many scales on a single axis (which is a requirement for me).

The fundamental issue is that I need the ability to apply a different scale
to each and every individual series on a cartesian chart, which means at a
bare minimum that each cartesian visualization will need to pass the series
to get the scale.

Now with all that negative stuff out of the way I am very interested to
hear what you come up with, and I too will spend time thinking about how I
could refactor this stuff so it doesn't affect cartesian and the
visualizations so much.

Another bit of "good" news in all of this is that line, scatter, column,
and bar (non-stacked) are the only chart types I would ever need to do this
for -- everything else (that I can think of) can't support multiple scales
or I don't need to support multiple scales with. Therefore, I do think I
have fully completed the use-case and these are the only visualizations
that need to have 'multiple scale smarts'. So while it is true that
multiple scales have leaked into the visualizations -- its not all of the
visualizations -- its not even apart of any new visualizations its just the
visualizations I have modified. Of course this does nothing for your
concerns about the impact on cartesian...

Now as a starting point I could imagine making a subclass of cartesian
called complexCartesian (which would be used in place of cartesian) which
adds in the multi-scaling (and right y axis) code. This though still
leaves all of the changes to the visualizations and the axis-scale-factory,
and the inclusion of the multi-linear-y-axis file.

I am still drawing a blank though about how to get around the conundrum of
visualizations which know nothing about multiple scales that support
multiple scales -- the only way I can think of to do that is to make it so
that the y function is a method on cartesian (or complexCartesian) which
takes as arguments the point (array element) to plot AND the series name OR
to make sure that the point always contains the series name plus this new y
method would also have to take a function which describes how to combine
d.y, d.y0 etc etc into a value to pass to a scale function. Something like
this

complexCartesian.supportsMultipleScales = function() { return true; }

complexCartesian.y = function(d, modDfn) {
var which = this.axisFor(d);
return this.which + 'Scale';
}

line.y = _.bind(function(d) {
return this.y(d, function(d) { return d.y + (d.y0 || 0); }) + 0.5
}, this)

line.render = function() {
var data = ....
if (this.supportsMultipleScales())
data = data.map(function(series) { return
series.data.map(function(pt) { pt.name = series.name return pt; })})
........
}

Therefore I guess if you are willing to bear with the changes to
axis-scale-factory and to include my multi-linear-y-axis file I could strip
most of the changes out of cartesian.js into complex-cartesian.js. I could
also then modify the visualizations I mentioned to always include the
series name in the point data if and only if complexCartesian is
constructed.

That is the best that I can think of at the moment.

On Mon, Mar 16, 2015 at 6:16 PM, Jaime del Palacio <notifications@github.com

wrote:

Hey, so far my thinking is that the multiple axes is not something I
specially want to support (bad practice and all that :), but I want to make
it easy for you to have a specific extension that allows for that. My
concern with the current implementation is that the concept of the multiple
axes get's leaked into every part of cartesian() and the visualizations. So
give me a few days to come up with a solution that does not involve so much
dependencies in the axes and I'll let you know. How does that sounds?

OTHO, I made a few changes to master that relate to jshint and code
standards, and also fixed some tests that were broken.


Reply to this email directly or view it on GitHub
#209 (comment).

@jaimedp
Copy link
Contributor

jaimedp commented Mar 18, 2015

Hi Billy,

After some refactor of cartesian(), I think I found a way of creating a multiAxes component that would work for multiple axes on the left or right sides, without leaking into cartesian or the visualizations. Take a look at branch 'axes' (https://github.com/forio/contour/tree/axes).

I added the code for the multiAxes component in the file examples/multi-axes.html

I'm sure there's still things to work out to make it resize and re-render correctly. Let me know what you think.

@wasbridge
Copy link
Contributor Author

This structure does show promise and definitely solves my right axis
problem (esp when finished to support resizing, new data etc etc).

I think this solution could also be used as a basis to solve my multiple
scales on a single axis problem, but I think to do that either the
axis-scale-factory has to be opened up to external components OR my
multi-linear-y-axis needs to be a part of the contour project.

I have included an image of what I mean when I say multiple scales on a
single axis.

[image: Inline image 1]

On Wed, Mar 18, 2015 at 1:52 AM, Jaime del Palacio <notifications@github.com

wrote:

Hi Billy,

After some refactor of cartesian(), I think I found a way of creating a
multiAxes component that would work for multiple axes on the left or right
sides, without leaking into cartesian or the visualizations. Take a look at
branch 'axes' (https://github.com/forio/contour/tree/axes).

I added the code for the multiAxes component in the file
examples/multi-axes.html

I'm sure there's still things to work out to make it resize and re-render
correctly. Let me know what you think.


Reply to this email directly or view it on GitHub
#209 (comment).

@wasbridge
Copy link
Contributor Author

screen shot 2015-03-18 at 11 07 28 am

@jaimedp
Copy link
Contributor

jaimedp commented Mar 18, 2015

Take a look at my latests commits in the axes branch, I think that change would allow you add any axis constructor to the _.nw.axes namespace and then use it as a type for the axisFactory, that way you can specify it in the type for the multi axis component...

@wasbridge
Copy link
Contributor Author

Yes -- that works.

Do you want me to make this all work with resizing, and setting new data or
is that something you are planning on doing?

On Wed, Mar 18, 2015 at 4:28 PM, Jaime del Palacio <notifications@github.com

wrote:

Take a look at my latests commits in the axes branch, I think that change
would allow you add any axis constructor to the _.nw.axes namespace and
then use it as a type for the axisFactory, that way you can specify it in
the type for the multi axis component...


Reply to this email directly or view it on GitHub
#209 (comment).

@jaimedp
Copy link
Contributor

jaimedp commented Mar 18, 2015

Ok, let me merge that branch into master and then you can take it from the to do the work on the 'multiAxies' component. How's that?

@wasbridge
Copy link
Contributor Author

Yes -- that works for me. I assume from the way you are talking that you
want to keep all of the multi axis stuff out of the actual contour project,
instead making it something that I maintain externally? Or have I
mis-understood your intent? I am happy to do it either way.

On Wed, Mar 18, 2015 at 5:24 PM, Jaime del Palacio <notifications@github.com

wrote:

Ok, let me merge that branch into master and then you can take it from the
to do the work on the 'multiAxies' component. How's that?


Reply to this email directly or view it on GitHub
#209 (comment).

@jaimedp
Copy link
Contributor

jaimedp commented Mar 18, 2015

Yes, that's exactly the idea. the multi-axes would be an external component that you can keep separate from the core contour repo, the idea is at some point to have a repository for people to contribute those external components if they like.

@jaimedp jaimedp closed this Dec 22, 2015
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.

None yet

2 participants