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

Implement strict autotypenumbers #5240

Merged
merged 25 commits into from Nov 4, 2020
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 29, 2020

Fixes #5250 and,
Resolves #5195 by introducing two new attributes at layout and axis levels.

TODO:

  • add tests

@plotly/plotly_js

if(linearOK(array)) return 'linear';
else return '-';
if(linearOK(array, convertNumeric)) return 'linear';
else return convertNumeric ? '-' : 'category';
Copy link
Contributor

@alexcjohnson alexcjohnson Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to leave else return '-', for empty arrays and arrays with only null entries, and instead include convertNumeric in the category(array) call, ie replace:

else if(Lib.cleanNumber(ai) !== BADNUM) curvenums++;

with something like:

else if(convertNumeric ? Lib.cleanNumber(ai) !== BADNUM : typeof ai === 'number') curvenums++;

Copy link
Contributor Author

@archmoj archmoj Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Done in b73d395.

src/plots/layout_attributes.js Outdated Show resolved Hide resolved
src/plots/layout_attributes.js Outdated Show resolved Hide resolved
@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Nov 3, 2020

OK so @alexcjohnson and I chatted about this and the attribute name should be autotypenumbers and it should be an enum with values "accept strings" or "strict", default value is "accept strings".

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Nov 3, 2020

@archmoj pointed out just now that while we're at it here we could stop treating the number 2000 as a date for autotype purposes. Basically: as far as autotype is considered, in strict mode dates must be strings, numbers must be numbers, and categories must not be numbers.

I like this idea, and I think 'strict' is still a good value, but I think then we should change 'accept strings' to 'convert types' or 'cast types'.

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Nov 3, 2020

is it just the number 2000 or something like "integers below 3000" ?

@archmoj archmoj changed the title Implement convertnumeric for axes and inherit from layout.axesconvertnumeric Implement strict autotypenumbers Nov 3, 2020
@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Nov 3, 2020

I'd have to check exactly, but I believe it's at least all 4-digit integers, possibly also 2-digit integers.

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Nov 3, 2020

I'd have to check exactly, but I believe it's at least all 4-digit integers, possibly also 2-digit integers.

Also 2-digit integers: Demo.

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Nov 4, 2020

@nicolaskruchten @alexcjohnson
This PR is now ready to review.
Thank you!

src/plots/layout_attributes.js Outdated Show resolved Hide resolved
src/traces/box/defaults.js Outdated Show resolved Hide resolved
src/traces/box/defaults.js Outdated Show resolved Hide resolved
Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

💃 💯

@archmoj archmoj merged commit 4e09ad3 into master Nov 4, 2020
9 checks passed
@archmoj archmoj deleted the disable-convertnumeric-in-autotype branch Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants