The Wayback Machine - https://web.archive.org/web/20210907115112/https://github.com/plotly/plotly.js/pull/5615
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

Add a basic smith chart #5615

Open
wants to merge 102 commits into
base: master
Choose a base branch
from
Open

Add a basic smith chart #5615

wants to merge 102 commits into from

Conversation

@waxlamp
Copy link

@waxlamp waxlamp commented Apr 28, 2021

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
Copy link
Collaborator

@archmoj archmoj commented May 19, 2021

@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
Copy link
Author

@waxlamp waxlamp commented May 19, 2021

@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
Copy link
Contributor

@jackparmer jackparmer commented May 21, 2021

... 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
Copy link
Author

@waxlamp waxlamp commented May 21, 2021

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

@waxlamp waxlamp force-pushed the arclamp:smith branch from 1ebf219 to 38dcaca Jun 22, 2021
@waxlamp waxlamp marked this pull request as ready for review Jun 22, 2021
@archmoj
Copy link
Collaborator

@archmoj archmoj commented Jun 25, 2021

@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!

@waxlamp waxlamp force-pushed the arclamp:smith branch from 38dcaca to c821377 Jun 28, 2021
@waxlamp
Copy link
Author

@waxlamp waxlamp commented Jun 28, 2021

@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
Copy link
Collaborator

@archmoj archmoj commented Jul 7, 2021

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

@archmoj
Copy link
Collaborator

@archmoj archmoj commented Aug 17, 2021

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

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

var real = layoutOut.smith.realaxis;

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

This comment has been minimized.

@archmoj

archmoj Aug 17, 2021
Collaborator

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

This comment has been minimized.

@waxlamp

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.

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

This comment has been minimized.

@archmoj

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

This comment has been minimized.

gridwidth: axesAttrs.gridwidth
}, 'plot', 'from-root');

var axisTickAttrs = overrideAll({

This comment has been minimized.

@archmoj

archmoj Aug 17, 2021
Collaborator

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}),

This comment has been minimized.

@archmoj

archmoj Aug 17, 2021
Collaborator

Please test cliponaxis: true in the image test.

This comment has been minimized.

@waxlamp

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?

This comment has been minimized.

@alexcjohnson

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

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

This comment has been minimized.

@archmoj

archmoj Aug 17, 2021
Collaborator

connectgaps is working?

This comment has been minimized.

@waxlamp

waxlamp Aug 18, 2021
Author

Yes. Added a test in aa13b91.

textposition: scatterAttrs.textposition,
textfont: scatterAttrs.textfont,

fill: extendFlat({}, scatterAttrs.fill, {

This comment has been minimized.

@archmoj

archmoj Aug 17, 2021
Collaborator

Do we have tests for fill?

This comment has been minimized.

@waxlamp

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?

This comment has been minimized.

@waxlamp

waxlamp Aug 18, 2021
Author

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


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

This comment has been minimized.

@archmoj

archmoj Aug 17, 2021
Collaborator

Please add tests for these texttemplate options.

This comment has been minimized.

@waxlamp
Copy link
Author

@waxlamp waxlamp commented Aug 18, 2021

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

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

ffd19e7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants