Add a basic smith chart by waxlamp · Pull Request #5615 · plotly/plotly.js

waxlamp · 2021-04-28T18:13:29Z

This draft PR shows a WIP approach to adding a Smith chart to Plotly. This consists of a new scattersmith trace and a new smith chart type that work together. scattersmith and smith are copies of scatterpolar and polar that I have slowly mutated in place to implement Smith chart semantics.

TODOs:

transform the radial axis into the "real" axis of the smith chart
change internal names to match smith chart semantics
create proper settings in scattersmith config
update how axis "tick marks" are configured (maximum value, rather than number of lines)
Rename radialaxis and angularaxis to realaxis and imaginaryaxis respectively
Add jasmine tests for scattersmith and smith (based on scatterpolar and polar)

archmoj · 2021-05-19T21:51:57Z

@waxlamp Thanks for the PR.
Would you please provide PR description as well as a list of TODOs?
Also if would be nice if you could fix syntax and lint issues using

npm run test-syntax && npm run lint

then pull & merge upstream/master into this branch.

waxlamp · 2021-05-19T21:54:59Z

@waxlamp Thanks for the PR.
Would you please provide PR description as well as a list of TODOs?
Also if would be nice if you could fix syntax and lint issues using
npm run test-syntax && npm run lint
then pull & merge upstream/master into this branch.

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.

jackparmer · 2021-05-21T00:26:29Z

... 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.

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!

waxlamp · 2021-05-21T16:38:45Z

... 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.
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 😸

archmoj · 2021-06-25T17:12:15Z

@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.
So I think you need to fetch upstream/master and rebase you branch on top of it and then force push.
That would be the last force push before we could start reviewing.
Thanks!

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.

waxlamp · 2021-06-28T19:24:07Z

@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.
So I think you need to fetch upstream/master and rebase you branch on top of it and then force push.
That would be the last force push before we could start reviewing.
Thanks!

Thanks for the guidance--I just did this, so we should be able to discuss later today. Looking forward to it!

archmoj · 2021-07-07T18:46:53Z

@waxlamp would you please fetch upstream master and merge (or rebase).
Thanks!

archmoj · 2021-08-17T14:24:43Z

Please add one of the scattersmith mocks to this list testing:

plotly.js/test/jasmine/assets/mock_lists.js

Line 40 in 587d86c

['scattercarpet',require('@mocks/scattercarpet.json')],

archmoj · 2021-08-17T14:38:55Z

+ var real = layoutOut.smith.realaxis;
+
+ expect(imag.type).toBe('linear');
+ expect(real.type).toBe('linear');


The type should be expected to be undefined for these axes not linear.

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.

archmoj · 2021-08-17T14:47:20Z

+ expect(imag.type).toBe('linear');
+ expect(real.type).toBe('linear');
+ });
+});


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){

See c1613b2.

archmoj · 2021-08-17T15:53:14Z

+ gridwidth: axesAttrs.gridwidth
+}, 'plot', 'from-root');
+
+var axisTickAttrs = overrideAll({


Some of these may not be supported.
Let's test all the working features in the image test.

archmoj · 2021-08-17T15:57:02Z

+ connectgaps: scatterAttrs.connectgaps,
+
+ marker: scatterAttrs.marker,
+ cliponaxis: extendFlat({}, scatterAttrs.cliponaxis, {dflt: false}),


Please test cliponaxis: true in the image test.

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?

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

archmoj · 2021-08-17T16:01:09Z

+ smoothing: lineAttrs.smoothing,
+ editType: 'calc'
+ },
+ connectgaps: scatterAttrs.connectgaps,


connectgaps is working?

Yes. Added a test in aa13b91.

archmoj · 2021-08-17T16:15:57Z

+ textposition: scatterAttrs.textposition,
+ textfont: scatterAttrs.textfont,
+
+ fill: extendFlat({}, scatterAttrs.fill, {


Do we have tests for fill?

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?

Nonetheless, it does seem to work, and I added a test: 383b6f1

archmoj · 2021-08-17T16:26:16Z

+
+ text: scatterAttrs.text,
+ texttemplate: texttemplateAttrs({editType: 'plot'}, {
+ keys: ['re', 'im', 'text']


Please add tests for these texttemplate options.

Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>

waxlamp · 2021-08-18T23:19:04Z

Please add one of the scattersmith mocks to this list testing:
plotly.js/test/jasmine/assets/mock_lists.js
Line 40 in 587d86c
['scattercarpet',require('@mocks/scattercarpet.json')],

ffd19e7

archmoj · 2021-09-27T18:49:19Z

Thanks for the contribution on this PR.
💃
Merging to smith-prototype dev branch which is on par with master branch at this moment..

archmoj added the type: new trace type label Apr 29, 2021

jackparmer mentioned this pull request May 1, 2021
Support plotting to Smith Charts #4074
Closed

nicolaskruchten linked an issue May 1, 2021 that may be closed by this pull request
Support plotting to Smith Charts #4074
Closed

waxlamp force-pushed the smith branch from 1ebf219 to 38dcacaCompare June 22, 2021 19:10

waxlamp marked this pull request as ready for review June 22, 2021 19:21

waxlamp added 16 commits June 28, 2021 15:22

Add a dummy scattersmith trace
9492c99
This is just a copy of scatterpolar with a changed name.

Modify scattersmith trace to use re and im data series
2ac5084
This commit also adds a `smith` plot type, which is a copy of polar, with minimal changes needed to host the `scattersmith` trace type.

Perform gamma transform (with fixed z0 of unity)
9366507

Force smith chart to use radial range [0, 1]
e45b7b8

Add a WIP function to draw circular "real" lines
6425813

WIP
f7cd0b0

Display real ticks at 0.2, 0.5, 1.0, 2.0, 5.0
3914dc0

Draw grid lines for imaginary axis
97a2224

Draw imaginary axis line properly
e2f6abd

Clip imaginary axis grid lines to correct angular bounds
7cbf9bc

Draw imaginary axis tick labels
7a6f86c

Draw tick labels for real axis
edee054

Adjust placement of real axis tick labels
eeba96d
Also cleaned up code a little bit.

Automatic lint fixes
be8a250

Restrict points to within the smith chart area
17e60c0

Fix non-automatic lint errors
c821377

waxlamp force-pushed the smith branch from 38dcaca to c821377Compare June 28, 2021 19:23

Add mock and baseline for smith chart
7ee57e6