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

fixed a bug where fragmented categorical data crashed the plot function #18

Merged
merged 2 commits into from
Apr 27, 2020

Conversation

sg-s
Copy link
Contributor

@sg-s sg-s commented Apr 24, 2020

This patch fixes a bug where a categorical array with some categories not being represented crashed the plot function. This was because the plot function looped over the categories, instead of the unique values in the categorical array.

@bastibe
Copy link
Owner

bastibe commented Apr 24, 2020

Cool, thank you!

I see you also changed the X-Tick-Rotation. I'd prefer that not to change, as current users might rely on existing functionality not to change.

You also changed the spelling of a number of set() function calls. Is there a specific reason for that?

@sg-s
Copy link
Contributor Author

sg-s commented Apr 24, 2020

Hi @bastibe I understand about the XTickRotation, I modified the PR to leave it unchanged

As far as I know, the spellings for axis properties use capitalization for every word, as defined in this page:

https://www.mathworks.com/help/matlab/ref/matlab.graphics.axis.axes-properties.html

I believe the format you used may break in the future.

@sg-s
Copy link
Contributor Author

sg-s commented Apr 24, 2020

After some investigation, I learnt this:

the reason your syntax works (with all lower case xtick) is because you're using the old way of modifying axis properties (using set...)

The "new" way does not support all lower-case syntax, e.g.:

ax = gca;
ax.xtick = ... will error
ax.XTick = ...  will work

@bastibe
Copy link
Owner

bastibe commented Apr 27, 2020

Thank you for your investigation.

I fear that the transition from get/set to object properties will be a painful one; Although Mathworks being Mathworks I expect them to keep the old spellings around for a long time.

Regardless, I'm fine with either spelling, so we might as well use the capitalized ones. In truth, I would actually prefer the property syntax, but that only works as of 2017 or so, and too many people are still on older versions, so we can't do that (yet).

Anyway. This looks good to me. Is it ready to merge, do you think?

@sg-s
Copy link
Contributor Author

sg-s commented Apr 27, 2020

should be good!

@bastibe bastibe merged commit 7839fd0 into bastibe:master Apr 27, 2020
@bastibe
Copy link
Owner

bastibe commented Apr 27, 2020

Thank you very much for your contribution, then!

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

Successfully merging this pull request may close these issues.

2 participants