Skip to content

MAINT: legacy-printing-mode preserves 1.13 float & complex str #10034

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

Merged
merged 1 commit into from
Nov 19, 2017

Conversation

ahaldane
Copy link
Member

Fixes #10029

This PR makes it so legacy mode preserves the truncation behavior for str(np.floatxx()), and the * behavior of complex scalars (eg (1+nan*j)) from numpy 1.13.

The implementation was just to cut-n-paste the old str/repr code from numpy 1.13, with minor modifications to fit it in. Then I add a C global variable which is set in legacy mode, and then use the old str/repr code if it is set.

@ahaldane
Copy link
Member Author

Would be nice to merge #10030 first, to rebase onto.

@ahaldane ahaldane added this to the 1.14.0 release milestone Nov 15, 2017
@charris
Copy link
Member

charris commented Nov 18, 2017

#10030 is in, doesn't seem to need a rebase.

@ahaldane
Copy link
Member Author

It needs a small rework, I'll get it done soon.

@ahaldane ahaldane force-pushed the legacy_scalars branch 2 times, most recently from 3c20cb2 to 88e8fb3 Compare November 18, 2017 17:32
@ahaldane
Copy link
Member Author

Updated, should be good now.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Shame to reintroduce so much old code, but LGTM.

Do you think it would be a good idea toindent the entire section / wrap it in an #ifdef ALLOW_113_LEGACY or something to make it visually clear it's legacy code?

@ahaldane
Copy link
Member Author

Yeah maybe I should move all the old code to one big block and surround it by comments "START LEGACY FUNCTIONS" and "END LEGACY FUNCTIONS" instead of interleaving the old code.

@eric-wieser
Copy link
Member

That would be reasonable too. Could also move the definitions to a legacy_float_format.h.src file to include in scalartypes.c.src

@charris
Copy link
Member

charris commented Nov 18, 2017

That brings up the question of how we eventually get rid of the legacy code. I suppose we will need to deprecate that option at some point.

@ahaldane
Copy link
Member Author

All right, I moved it to one big block surrounded by comments.

@ahaldane
Copy link
Member Author

ahaldane commented Nov 18, 2017

Yes, but deprecating it shouldn't be too hard. Just add a warning anytime legacy mode is set durign the deprecation period, and after that raise an exception.

I think the scikit-learn devs said they only really needed the legacy mode for 1 or 2 release cycles, just enough time for them to update their tests. Edit: Here's the comment: (link)

@eric-wieser
Copy link
Member

eric-wieser commented Nov 18, 2017

Yeah, deprecation should be straightforward. The key thing is that we need at least one release to be compatible with 1.13, so that dependents don't have to update their tests in lock-step.

}
else {
char re[64], im[64];
if (npy_isfinite(val.real)) {
Copy link
Member

Choose a reason for hiding this comment

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

Four space indents. Did some hard tabs slip in?

@charris
Copy link
Member

charris commented Nov 19, 2017

LGTM apart from the 8 space indents.

@charris
Copy link
Member

charris commented Nov 19, 2017

I'm going to put this in and follow up with a STY PR. Thanks Allan.

@charris charris merged commit cddee1d into numpy:master Nov 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants