Skip to content

Conversation

@ojundt
Copy link
Contributor

@ojundt ojundt commented Dec 20, 2015

I was missing a way to pass 32-bit single precision floats to MessagePack since ruby only has 64-bit double precision floats.

With this change it is possible by simply wrapping any float (or in fact any numeric) ruby value in a new MessagePack::SinglePrecisionFloat instance. Thoughts?

The JRuby implementation is missing for now. Unfortunately I don't have time/experience for that part :/

@ojundt
Copy link
Contributor Author

ojundt commented Dec 20, 2015

Now also working with rubinius and JRuby :)

@iconara
Copy link
Member

iconara commented Dec 20, 2015

Wouldn't it be sufficient to just add #to_msgpack to your SinglePrecisionFloat? Do you really need this to be in the MessagePack library?

@ojundt
Copy link
Contributor Author

ojundt commented Dec 20, 2015

That's pretty much all I did + overwriting equality methods for testing purposes. If you add comment to the code where you think it's better placed somewhere else or left out, I would be happy to review it.

@iconara
Copy link
Member

iconara commented Dec 20, 2015

I meant: why do you need SinglePrecisionFloat to be in the MessagePack library when you can easily implement the functionality you need by implementing #to_msgpack in your own code?

In my opinion this does not belong in the MessagePack library. I don't mean that as a discouragement, I'm very happy that you want to contribute, and it might be the case that the other maintainers disagree with me.

I would much rather see something on the lines of my original JRuby implementation did: https://github.com/msgpack/msgpack-ruby/blob/master/ext/java/org/msgpack/jruby/Encoder.java#L186-L191 (i.e. detect when a floating point number can be represented using 32 bits).

It's the wrapper class (SinglePrecisionFloat) that doesn't feel right. You shouldn't need to "cast" your values like that when using MessagePack. If you need special handling you should implement your own #to_msgpack, everything else should just work and be invisible to the client code.

@ojundt
Copy link
Contributor Author

ojundt commented Dec 20, 2015

Really appreciate your input. I agree that the additional class feels a bit weird but maybe wrapper class isn't the good word here. It's actually just an implementation of the missing single precison float type in Ruby and that's how you should see it.

I don't see how I could force the serialization to the 32 bit format by simply implementing #to_msgpack without the additional type based switch in the packer. Can you give an example?

The automatic detection is certainly a nice optimization, assuming we can solve the platform dependency problems mentioned in #6 but it does not solve the problem that users cannot easily create single precision float values at the first place.

Maybe a combination of automatic detection and explicit typing is the right way to go. Assume the automatic detection works. All we needed then was something like a #to_single_precision method for Float which returns a new Ruby Float that can certainly be casted to a 32-bit float without loss. It would then be picked up by the automatic detection and serialized correctly. The #to_single_precision of course doesn't necessarily have to be part of this library. What do you think? Should the focus be put on the detection?

@tagomoris
Copy link
Member

I agree with @iconara. Introducing SinglePrecisionFloat seems too much for msgpack-ruby, because Ruby's built-in classes only have Float, which represents double-precision floating point value.
http://docs.ruby-lang.org/en/2.2.0/Float.html

What do you want to do?

  • represent single-precision floating point value in Ruby world? (post it to bugs.ruby-lang.org)
  • serialize Ruby's Float values into FLOAT32 with lost precision? (you can introduce your own new class with #to_msgpack method, and add method on Float to convert to it)
  • any others?

@tagomoris tagomoris changed the title Possibility to pass 32-byte single precision floats to MessagePack Possibility to pass 32-bit single precision floats to MessagePack Dec 21, 2015
@ojundt
Copy link
Contributor Author

ojundt commented Dec 21, 2015

@tagomoris Option 2 is what I want. Then #to_msgpack should look like this, right?

def to_msgpack(pk = nil)
   return MessagePack.pack(self, pk) unless pk.class == MessagePack::Packer
   buffer = pk.buffer
   buffer.write("\xCA")
   buffer.write([numeric].pack('g'))
   pk
end

Unfortunately the detour via #pack is quite slow. Would a compromise be to add #write_float32 to Packer similar to the existing #write_nil and such? Then the method could be simplified to:

def to_msgpack(pk = nil)
   return MessagePack.pack(self, pk) unless pk.class == MessagePack::Packer
   pk.write_float32(numeric)
end

@tagomoris
Copy link
Member

@ojundt The fastest implementation is to implement extension library in C for you own class, and use Packer#write with it. It calls rb_funcall just once for #to_msgpack to get serialized value of argument object... so not so bad in sight of performance.

@ojundt
Copy link
Contributor Author

ojundt commented Dec 21, 2015

I might be doing something wrong but if I try to return an already serialized float32 in #to_msgpack or write and already serialized float32 with Packer#write it gets serialized as a string. Not what I want. Could you give an example implementation in either Ruby or C?

I just pushed the new proposal without SinglePrecisionFloat btw. Got an error in the random test on travis, though. Is this a known issue?

@ojundt ojundt changed the title Possibility to pass 32-bit single precision floats to MessagePack Possibility to write 32-bit single precision floats to MessagePack Dec 21, 2015
@ojundt
Copy link
Contributor Author

ojundt commented Dec 21, 2015

After a good night sleep I really think that #write_float32 has its legitimate place in this library. As far as I can see the float32 format is the last missing MessagePack supported format that currently cannot be easily accessed for serialization with this library.

With #write_float32 people get a simple API to a basic MessagePack feature, that wasn't easily accessible before. AFAIK without this change the only way to actually serialize to the MessagePack float32 format is if I implement the float32 serialization myself as illustrated in my #to_msgpack example code before. That doesn't feel right either, does it? Knowing the header, knowing how much bytes to reserve and how to serialize it to Big Endian 32-bit float is so basic, it should be part of this library.

Then you are only left with the problem that Ruby does not have a 32-bit float type itself. So what type do we pass to the method?
When deserializing from the float32 MessagePack format we already type cast to a Ruby 64-bit float. Then why not the other way around and allow users to simply pass in a Ruby 64-bit float that gets casted within the library? Ofcourse you loose precision but since they would explicitly have to call #write_float32 it is a conscious decision and the API remains simple and fast.

@ojundt ojundt closed this Dec 22, 2015
@ojundt ojundt reopened this Dec 22, 2015
@ojundt ojundt closed this Dec 27, 2015
@ojundt ojundt reopened this Dec 27, 2015
@ojundt
Copy link
Contributor Author

ojundt commented Dec 27, 2015

@tagomoris I saw you had the same trouble with the rubinius tests so I adapted your commit to temporarily disable them.

Any thoughts on the new proposal?

@tagomoris
Copy link
Member

IMO, adding write_float32 looks not so bad. It provides the way to pack floats into less data if user need it.
It's not for almost all users, but we already have write_* methods.

@frsyuki How do you think about this idea?

@frsyuki
Copy link
Member

frsyuki commented Jan 6, 2016

Adding write_float32 sounds OK idea. let's merge.
The only reason why msgpack_packer_write_double always uses double-precision is that I don't know how to write portable code in C that checks a double value can be converted to float without loosing information ("without loosing information" means that casting float back to double restores the original double value). If we can write such code, write(0.5) should automatically use float32 format. Even if this automation is implemented, write_float32 still seems OK to have because a possible application might want to store Numeric always using float32 format even if some precision is lost.

frsyuki added a commit that referenced this pull request Jan 6, 2016
Possibility to write 32-bit single precision floats to MessagePack
@frsyuki frsyuki merged commit e45b36e into msgpack:master Jan 6, 2016
@frsyuki
Copy link
Member

frsyuki commented Jan 6, 2016

I merged.
@ojundt Thank you for your contribution!

@tagomoris
Copy link
Member

This change is released as v0.7.3 for all platforms.

@ojundt
Copy link
Contributor Author

ojundt commented Jan 7, 2016

Awesome. Thank you guys, too!

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.

4 participants