Changing Money-related variables from float to decimal.

Nucs commented 6 years ago

Summary

The value type for variables that hold a balance/portfolio or price/bid/ask should be decimal.

Pros

High accuracy when handling calculations with money (this is decimal's purpose and what it was created for).
decimal is the standard value type for handling money in all libraries I've worked with by now. (QuantConnect/Lean, sonvister/Binance, sefbkn/gdax.netcore)

Cons
Breaking Change that will require move to version 2.0.
There is a performance impact when using decimal, see reference.
A Decimaltakes 16 bytes vs Doublethat takes 8 bytes. essentially doubling the memory usage.

My Opinion The performance impact isn't small but tolerable. The accuracy is indeed required. The changes are breaking, therefore it will have to be released in a new major version release. Unless we plan to break the market's standard by being a special snowflake, I do not see a reason.

Would love to hear your opinions.

hunterfries commented 6 years ago

I agree. Will save the step of converting to decimal in our client apps. Pros outweigh the cons.

mathpaquette commented 5 years ago

@Nucs @hunterfries @jphiggs @mrut2pac thinking about changing from float to double

mathpaquette commented 5 years ago

just realized some edge cases like Berkshire Hathaway Inc. with a price of 313,270.00 so this is already exceeding the limit of 7 digits for a float.

Do we really need decimals ? I dont think so

mathpaquette commented 5 years ago

@Nucs float is actually 4 Bytes compare to 16 Bytes for a decimal. The thing is all our messages are immutable you ill never apply addition or other mathematical operators.

mathpaquette commented 5 years ago

@Nucs okay you were right I think. Lets aim for the standard decimal.... Im going to release that in v2 with Market Depth support :)

Nucs commented 5 years ago

I mean real high-frequency trading systems are written in C++, no-one should use C# for that kind of trading (performance-wise). So perf-wise moving to decimal is alright.

with a price of 313,270.00 so this is already exceeding the limit of 7 digits for a float.

I don't think the 7 digits refer to the real number but to the fraction precision. The range is huge.

mathpaquette commented 5 years ago

@Nucs my concern is mainly memory-wise not much performance. In my previous app, I needed to store in memory a bunch of Intervals data to perform calculation on the fly. Lets say the footprint was about 1.5Go now for the same thing it will be about 4 times the size without much additional value rather making sure that some edge cases like Berkshire can handle.

Nucs commented 5 years ago

@mathpaquette How about producing two nuget packages. One that uses decimal and an other float. The user can choose if he prefers compatibility with external libraries and precision or lower memory usage and faster performance. Add a definition FLOAT or DECIMAL to two separate build configuration (similarly to Debug and Release modes)

//at the top of the file
#if FLOAT
using valuetype = System.Single;
#elif DECIMAL
using valuetype = System.Decimal;
#endif

//use valuetype instead of float or double
valuetype somefunction();

mathpaquette commented 5 years ago

@Nucs could be but seems a little bit overkill :) What about replacing all float to double and providing ToDecimal for each messages ? Even SciChart doesnt support decimals if I'm not wrong.

Looks like a good compromise to me.

Not losing any precision
Keep memory usage as low as possible
Provides compatibility with existing library like (Lean etc.) when needed

Nucs commented 5 years ago

could be but seems a little bit overkill :)

Literally 10 minutes to set up projects and buildbot and another 15 minutes refactoring. You solve it how ever you like, I would do that this way.

mathpaquette commented 5 years ago

could be but seems a little bit overkill :)

Literally 10 minutes to set up projects and buildbot and another 15 minutes refactoring. You solve it how ever you like, I would do that this way.

Not overkill in a sense to setup but I never experienced that kind of duality for any libs I used in the past in C# which usability its a bad sign that kind of uniqueness if you know what I mean.

mathpaquette commented 5 years ago

could be but seems a little bit overkill :)

Literally 10 minutes to set up projects and buildbot and another 15 minutes refactoring. You solve it how ever you like, I would do that this way.

We are just discussing right now, I'm open-minded to what people are saying.

mrut2pac commented 5 years ago

I like the idea of having both float and decimal.

mathpaquette commented 5 years ago

Same, the design is so flexible we can support it easily. Just boilerplate code to add... I think at the factory level will be the best...

On Oct 20, 2019, at 9:32 PM, mrut2pac notifications@github.com wrote:

I like the idea of having both float and decimal.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mathpaquette/IQFeed.CSharpApiClient/issues/4?email_source=notifications&email_token=ABBICN3C7W22ZZAW5BIRZDDQPUBCNA5CNFSM4FHZVMKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBYZWAQ#issuecomment-544316162, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABBICN7A723C5BWSAZHG5Y3QPUBCNANCNFSM4FHZVMKA.

mathpaquette commented 4 years ago

guys, what do you think of this approach.... sounds a little bit overkill for me... but it works fine... https://github.com/mathpaquette/IQFeed.CSharpApiClient/commit/15bd95e8e3e2914ebc68b2d5952525ee8dbebebc

the order way, as @Nucs suggested many times if to go with decimals and to provide ToFloat ToDouble extensions

mathpaquette commented 4 years ago

took some time but finally supporting float, double and decimal out of the box.

mathpaquette commented 4 years ago

Version 3 will drop support for generic message handlers meaning doubles will become default across the library no matter what for simplicity and performance reasons.

mathpaquette / IQFeed.CSharpApiClient