-
Notifications
You must be signed in to change notification settings - Fork 11
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
feature: implied odds calculator #6
Comments
I think it's a useful addition. A couple of comments:
|
Yep, all that makes sense to me. I'm not particularly comfortable in rust yet, but I'll have a crack at it for the experience. Would you like to keep a python implementation available too? |
Your question is actually more nuanced than I immediately thought. My immediate reaction was that we should have a Python optimiser as well so that we match the situation with However, for as opposed to a general purpose one While I completely understand why you've done it, implementing a general purpose solver in the package as you have done in your branch doesn't sit entirely well with me but then neither does introducing a heavy weight dependency to the package such as scipy Reflecting on the history of the package development, I think that probably the only reason the Python optimiser implementation remains is because of backwards compatibility/uncertainty over the newer Rust one. Given that I've already introduced Rust code to the package, I think it makes sense that for entirely new functionality there's no need to replicate code in both Python and Rust and we should just have the optimiser implemented in Rust. I'd also be very happy to introduce a dependency at the Rust level on a crate that provides a general purpose optimiser although I'm not sure if there is a canonical choice here. Perhaps https://github.com/argmin-rs/argmin. Note that for reasons I cannot currently recall, in one of my other packages I simply ported |
I think easiest would be to copy that code straight in here then, would that suit? Where are you heading with the |
Copying it in would be fine - certainly I don't personally currently have the capacity to pick an off the shelf alternative or confirm that the port is required if none of them provide exactly the required functionality I haven't made any progress with those TODOs but I don't think they'll create any problems in this case |
In the notes for the public function that wraps that scipy minimization func (ref):
... and the argmin crate has a Brent implementation, so I've gone ahead and used that. I'll be spending more time on it as my rust is pretty raw, but feel free to comment on it at any point if you have the time (https://github.com/peterschutt/shin/pull/1/files) - but also no pressure. |
A natural counterpart to probability inference would be odds inference, i.e., output a set of prices for a given set of probabilities (that sum to 1) for a target overround/margin, where the sum of the inverse output prices would = 1 + target overround/margin.
Would you be interested in including something like this in the library?
R's implied package does this, and I've put together a very raw python implementation based from that which you can see diff'd against the typing branch here: https://github.com/peterschutt/shin/pull/1/files.
The text was updated successfully, but these errors were encountered: