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

Deprecate hex2num and num2hex #22088

Merged
merged 3 commits into from
Aug 10, 2017
Merged

Deprecate hex2num and num2hex #22088

merged 3 commits into from
Aug 10, 2017

Conversation

simonbyrne
Copy link
Contributor

See #22031. I'm open to suggestions for the deprecation message for hex2num.


@deprecate num2hex(x::Union{Float16,Float32,Float64}) =
hex(reintepret(Unsigned, x), sizeof(x)*2)
@deprecate num2hex(n::Integer) = hex(n, sizeof(n)*2)
Copy link
Contributor

Choose a reason for hiding this comment

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

at-deprecate doesn't need the =, right?

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 point.

@kshyatt kshyatt added the deprecation This change introduces or involves a deprecation label May 27, 2017
@rfourquet
Copy link
Member

rfourquet commented May 27, 2017

Instead of getting rid of those functions, I would prefer to transfer the functionality somewhere else. I didn't know them until few days ago, but I find num2hex in particular useful, as an alternative to bits which is harder to read (because counting a lot of 0s and 1s on the screen is difficult. So I propose as the path forward:

  1. Fix the bugs in those functions (check overflow in hex2num (fix #22031) #22039)
  2. deprecate (EDIT: cf. RFC: deprecate hex2num and num2hex, alternative to #22088 #22203), but incorporate num2hex into bits via an additional argument (bits(x, hex=true)), and hex2num maybe in parse (cf. e.g. coalesce (bytes2hex, num2hex, hex) and (bin, bits, etc.) #14418 (comment)).

@ararslan
Copy link
Member

I agree that the functionality should still be easily accessible. Having to do reinterpret(Float64, parse(UInt64, s, 16)), for example, is a little unfortunate.

@simonbyrne
Copy link
Contributor Author

I agree that the functionality should still be easily accessible. Having to do reinterpret(Float64, parse(UInt64, s, 16)), for example, is a little unfortunate.

I disagree: it's not clear exactly what the use case of this function is, and as discussed in #22031 it seems to encourage poor practices.

@rfourquet
Copy link
Member

hex2num acts as the opposite of bits, so if one is available in Base, the other should also be available IMHO. As Julia is a language where we can play easily with bits (and even create primitive bit types!), I feel that those functions are a nice fit in the base library. Everyone seems to agree that the names should be changed though.

@joa-quim
Copy link

Everyone seems to agree that the names should be changed though.

I do not. Those are the names used in Matlab (not by coincidence I guess) and I feel that a change will only bring pain to Matlab users for no real gain.

@tkelman
Copy link
Contributor

tkelman commented May 30, 2017

matlab doesn't have dispatch, so a lot of its API naming choices aren't that useful in terms of what we should do (str2num anyone?)

@stevengj
Copy link
Member

stevengj commented Jun 6, 2017

@rfourquet, if you just want to read the hex format of a floating-point number, why not use reinterpret?

julia> reinterpret(UInt64, 3.7)
0x400d99999999999a

If you really need it as a string you can always use repr or show.

I have yet to see a defensible practical usage of hex2num/num2hex that isn't better to do via reinterpret. bits is different because there is no native Julia type that prints as binary.

@rfourquet
Copy link
Member

Yes you are right. Probably when I like to see the bits, in hex or binary form, I think "bits" rather than "reinterpret" in my head ;-) Still, reinterpret needs you to specify an unsigned type with the correct width.

@stevengj
Copy link
Member

stevengj commented Jun 6, 2017

We could always add a reinterpret(Unsigned, x) method that automatically infers the width.

@rfourquet
Copy link
Member

That's exactly how I implemented bits(x, hex) in #22203 ! Still, I find it convenient. And it can be needed also programatically (not only to see it printed on screen).

@stevengj
Copy link
Member

stevengj commented Jun 6, 2017

The question remains, what real application needs a hex form of a Float number as an actual string object that isn't served at least as well by reinterpret?

As I mentioned in #22031, the only usage of hex2num/num2hex that I found in any package would be greatly improved by switching to reinterpret.

@rfourquet
Copy link
Member

rfourquet commented Jun 6, 2017

I'm all for deprecating num2hex, but I feel it doesn't overload Base too much to add an optional argument to bits to have the number displayed in hex instead of binary.

@stevengj
Copy link
Member

stevengj commented Jun 6, 2017

I don't see the value of complicating bits in this way, vs. just recommending reinterpret(Unsigned, x). Why have two ways to do this?

@stevengj stevengj added this to the 1.0 milestone Aug 3, 2017
@stevengj
Copy link
Member

stevengj commented Aug 3, 2017

Adding 1.0 milestone since we should really get a decision on this.

@ararslan ararslan added the needs decision A decision on this change is needed label Aug 3, 2017
@JeffBezanson
Copy link
Member

Triage is in favor. Let's rebase and merge.

@JeffBezanson JeffBezanson removed the needs decision A decision on this change is needed label Aug 10, 2017
@simonbyrne
Copy link
Contributor Author

done.

@JeffBezanson JeffBezanson merged commit 59e434d into master Aug 10, 2017
@JeffBezanson JeffBezanson deleted the sb/hex2num branch August 10, 2017 21:33
return reinterpret(Float64, parse(UInt64, s, 16))
end

@deprecate num2hex(x::Union{Float16,Float32,Float64}) hex(reintepret(Unsigned, x), sizeof(x)*2)
Copy link
Member

Choose a reason for hiding this comment

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

reintepret -> reinterpret

simonbyrne added a commit that referenced this pull request Aug 11, 2017
Introduce by #22088. Found by @fredrikekre
@simonbyrne simonbyrne mentioned this pull request Aug 11, 2017
ararslan pushed a commit that referenced this pull request Aug 12, 2017
rfourquet added a commit that referenced this pull request Aug 18, 2017
@ViralBShah
Copy link
Member

This needs an entry in NEWS.

@ararslan ararslan added the needs news A NEWS entry is required for this change label Oct 20, 2017
@ararslan ararslan removed the needs news A NEWS entry is required for this change label Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants