-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Carpet plot rebase #1595
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
Carpet plot rebase #1595
Changes from all commits
eee837f
5a0b788
89e095e
e1a0417
620cd43
728a6f4
c2994e7
fe9850c
d1f2258
4bcf438
6b896d8
b8c71e1
e4971ef
c98553a
71b96f3
bc1c5c8
9c76d7a
85d6b8a
54f092d
d9cb011
f1e8aee
bc0f890
c2a7092
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* Copyright 2012-2017, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
module.exports = require('../src/traces/carpet'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* Copyright 2012-2017, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
module.exports = require('../src/traces/contourcarpet'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* Copyright 2012-2017, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
module.exports = require('../src/traces/scattercarpet'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/** | ||
* Copyright 2012-2017, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
/* | ||
* Ensures an array has the right amount of storage space. If it doesn't | ||
* exist, it creates an array. If it does exist, it returns it if too | ||
* short or truncates it in-place. | ||
* | ||
* The goal is to just reuse memory to avoid a bit of excessive garbage | ||
* collection. | ||
*/ | ||
module.exports = function ensureArray(out, n) { | ||
if(!Array.isArray(out)) out = []; | ||
|
||
// If too long, truncate. (If too short, it will grow | ||
// automatically so we don't care about that case) | ||
out.length = n; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. huh, never knew you could resize this way! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Best I can tell, it's actually robust and supported across platforms. It's possible it does weird things internally (like not actually de-allocating that memory. I saw a while ago some discussion of the various internal string apis and string data structures in js. Hint: it's not straightforward). The main goal was to just overwrite the carpet spline stuff rather than allocating everything from scratch every time since that adds up to lots of garbage collection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/length
That's right, [..] sets or returns [..]. It's not a read-only property like one might assume. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (best-case scenario though is that it actually truncates in-place without actually copying. I mean maybe copies, but |
||
|
||
return out; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ var extendFlat = require('../../../lib/extend').extendFlat; | |
|
||
|
||
module.exports = { | ||
visible: axesAttrs.visible, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think you had to put it in there so that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, correct. Was contemplating the best option as was working back through that. Correct though, it was just to make them not fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the right answer is not to remove the attribute and avoid supplying that default in that case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in #1599 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh wow, nice! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #1599 is now merged in this PR. |
||
showspikes: { | ||
valType: 'boolean', | ||
role: 'info', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why you did it this way, but it seems a little redundant, we're making a new node only to clone it in the global tester, then we delete them both. If this ends up getting called a lot, for performance we may want to break up
drawing.bBox
in such a way that this element can get created in the global tester in the first place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I didn't appreciate that the tester was also cloning it. And to add to that, it looks like I only ended up using it in one place. 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but on the plus side, to answer your question, at least it doesn't end up getting called a lot 😄 )