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
Collaborator

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

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.

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
@nicolaskruchten
Copy link
Member

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
Collaborator

@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

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
Collaborator

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 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 commented Nov 4, 2020

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

src/traces/box/defaults.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 💯

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