Add a basic smith chart #5615
Add a basic smith chart #5615
Conversation
@waxlamp Thanks for the PR.
then pull & merge |
I marked this PR as "draft" specifically because it's really not ready for review yet. I can still run the syntax fixing and master merge steps if you like, but my plan was to continue working in this "dirty" mode, then at the end clean up the commits and perhaps file them via a new PR branch (closing this one) on which I'd pay much closer attention to the software standards for a ready-to-merge pull request. Let me know how you'd like me to proceed. |
Sounds fine @waxlamp . It might still be nice to see a list of TODOs to break down and understand what's left. Looking forward to discussing tomorrow! |
Completely forgot to address the TODO suggestion from @archmoj, sorry! Yes I will add some TODOs |
@waxlamp FYI the old image test system is replaced with a new one which would be needed in this PR i.e. to generate new baselines. |
This is just a copy of scatterpolar with a changed name.
This commit also adds a `smith` plot type, which is a copy of polar, with minimal changes needed to host the `scattersmith` trace type.
Also cleaned up code a little bit.
Thanks for the guidance--I just did this, so we should be able to discuss later today. Looking forward to it! |
@waxlamp would you please fetch upstream master and merge (or rebase). |
Please add one of the |
var real = layoutOut.smith.realaxis; | ||
|
||
expect(imag.type).toBe('linear'); | ||
expect(real.type).toBe('linear'); |
archmoj
Aug 17, 2021
Collaborator
The type
should be expected to be undefined for these axes not linear.
The type
should be expected to be undefined for these axes not linear.
waxlamp
Aug 18, 2021
Author
As far as I know, the axis types really are linear
. It may be an implementation detail, in which case I can simply remove these assertions from the test.
As far as I know, the axis types really are linear
. It may be an implementation detail, in which case I can simply remove these assertions from the test.
expect(imag.type).toBe('linear'); | ||
expect(real.type).toBe('linear'); | ||
}); | ||
}); |
archmoj
Aug 17, 2021
Collaborator
Let's copy & adapt these test from polar_test
:
it('should propagate axis *color* settings', function() {
it('should coerce hoverformat even for `visible: false` axes', function() {
it('should be able to reorder axis layers when relayout\'ing *layer*', function(done) {
it('should be able to toggle axis features', function(done) {
it('should clean up its framework, clip paths and info layers when getting deleted', function(done) {
it('should trigger hover/unhover/click/doubleclick events', function(done) {
Let's copy & adapt these test from polar_test
:
it('should propagate axis *color* settings', function() {
it('should coerce hoverformat even for `visible: false` axes', function() {
it('should be able to reorder axis layers when relayout\'ing *layer*', function(done) {
it('should be able to toggle axis features', function(done) {
it('should clean up its framework, clip paths and info layers when getting deleted', function(done) {
it('should trigger hover/unhover/click/doubleclick events', function(done) {
gridwidth: axesAttrs.gridwidth | ||
}, 'plot', 'from-root'); | ||
|
||
var axisTickAttrs = overrideAll({ |
archmoj
Aug 17, 2021
Collaborator
Some of these may not be supported.
Let's test all the working features in the image test.
Some of these may not be supported.
Let's test all the working features in the image test.
connectgaps: scatterAttrs.connectgaps, | ||
|
||
marker: scatterAttrs.marker, | ||
cliponaxis: extendFlat({}, scatterAttrs.cliponaxis, {dflt: false}), |
archmoj
Aug 17, 2021
Collaborator
Please test cliponaxis: true
in the image test.
Please test cliponaxis: true
in the image test.
waxlamp
Aug 18, 2021
Author
For smith charts, there's not much meaning in plotting values outside the main plot area--that is, cliponaxis
should always be true
and this shouldn't be an attribute in the trace. Can I remove it?
For smith charts, there's not much meaning in plotting values outside the main plot area--that is, cliponaxis
should always be true
and this shouldn't be an attribute in the trace. Can I remove it?
alexcjohnson
Aug 26, 2021
Contributor
cliponaxis
is more subtle than that - it's about what happens when a marker gets close to the subplot boundary: do we draw it partially, literally clipping the drawn shape to the subplot area, or do we continue drawing the complete shape until the data point itself exits the subplot area? See https://rreusser.github.io/plotly-mock-viewer/#cliponaxis_false
cliponaxis
is more subtle than that - it's about what happens when a marker gets close to the subplot boundary: do we draw it partially, literally clipping the drawn shape to the subplot area, or do we continue drawing the complete shape until the data point itself exits the subplot area? See https://rreusser.github.io/plotly-mock-viewer/#cliponaxis_false
smoothing: lineAttrs.smoothing, | ||
editType: 'calc' | ||
}, | ||
connectgaps: scatterAttrs.connectgaps, |
archmoj
Aug 17, 2021
Collaborator
connectgaps
is working?
connectgaps
is working?
textposition: scatterAttrs.textposition, | ||
textfont: scatterAttrs.textfont, | ||
|
||
fill: extendFlat({}, scatterAttrs.fill, { |
archmoj
Aug 17, 2021
Collaborator
Do we have tests for fill
?
Do we have tests for fill
?
waxlamp
Aug 18, 2021
Author
No, but my understanding is that fill doesn't really make sense in a smith chart. Should I keep it and write a test, or remove the attribute?
No, but my understanding is that fill doesn't really make sense in a smith chart. Should I keep it and write a test, or remove the attribute?
|
||
text: scatterAttrs.text, | ||
texttemplate: texttemplateAttrs({editType: 'plot'}, { | ||
keys: ['re', 'im', 'text'] |
archmoj
Aug 17, 2021
Collaborator
Please add tests for these texttemplate
options.
Please add tests for these texttemplate
options.
Co-authored-by: Mojtaba Samimi <[email protected]>
Co-authored-by: Mojtaba Samimi <[email protected]>
|
This draft PR shows a WIP approach to adding a Smith chart to Plotly. This consists of a new
scattersmith
trace and a newsmith
chart type that work together.scattersmith
andsmith
are copies ofscatterpolar
andpolar
that I have slowly mutated in place to implement Smith chart semantics.TODOs:
scattersmith
configradialaxis
andangularaxis
torealaxis
andimaginaryaxis
respectively