phillc73 / abettor

An R package for connecting to the online betting exchange Betfair, via their API-NG product, using JSON-RPC.
Other
51 stars 36 forks source link

placeOrders function does not include all fields #43

Closed Soccerama closed 3 years ago

Soccerama commented 3 years ago

The current implementation of the placeOrders function covers the required elements and some others but there are more fields that are not currently implemented. Adding each field into the function definition would be messy and deviates from Betfair's definition in the API docs. I'm developing an alternative methodology where the instruction list is passed in as a dataframe which seems to work after a lot of experimentation. The order list is built using some simple rules and only required fields need to be supplied. What is the best way to share this? Give it a new name like placeOrdersDF and add it to my fork? (Sorry I'm still a github noob).

Soccerama commented 3 years ago

I've done some more work on this function and initial testing looks good. Have just used a single call to place three bets, one one of each type (LIMIT, LIMIT_ON_CLOSE, MARKET_ON_CLOSE) on a single race. The first was a BACK and the other two are LAY bets. I've set up a template and a worked example to show how to use it, as the structure for the placeInstruction list is a bit more complex. Will keep working on the documentation and put it on my fork in the next few days.

Soccerama commented 3 years ago

I've added an alternative implementation of the placeOrders function, which I've called placeOrdersDF into my fork and it can be found here: https://github.com/Soccerama/abettor/blob/master/R/placeOrdersDF.R It is a bit different to the current implementation as it is fed market fields and then an instruction list in the form used by Betfair. It allows multiple bets of all three order types in a single instruction which is more efficient than the current function which only allows one type. It also includes all possible fields used by the Betfair operation which the current function doesn't (yes, some of these fields are a bit obscure but somebody must use them). It may look more complex as you have to build a data frame with a particular format but there is a worked example which hopefully shows that it's actually pretty straight forward. I would love to get feedback on the function. I'm now using it as it has complete flexibility and the dataframes are easy to construct within my code.

phillc73 commented 3 years ago

Thanks for taking the time to do this, and I'd be pleased to merge an enhanced placeOrders function, as long as it's backwards compatible with the existing placeOrders function, so that existing end user code doesn't break.

Looking at your enhanced function, a couple of questions/issues:

placeOrdersOps$params$instructions <-
      list(insts)

I don't see any other mention of insts in the function call. I wonder if this worked because you had a variable called insts in your local environment. Looking at the examples, you've built the insts dataframe outside the placeOrders function, but this will fail if the local user builds their instruction dataframe with any other variable name.

In the function you have a parameter called instructionList, but then that's not been included in placeOrderOps anywhere. It might just be a matter of updating the reference of insts inside the function with instructionList.

I think you have a typo in the main function name too. Code says:

SplaceOrders <-

Need to remove the S at the start.

phillc73 commented 3 years ago

Also, what's the main goal of the new function? Is it to place simultaneous bets of different types at the same time?

Current placeOrders is limited to one of three betType instructions, for each individual function call:

Is the goal of the new function to mix and match different bet types in a single call? If that's the goal, I'd like to do some more testing if your approach.

Are there other instructions that are also missing from the existing placeOrders function? If so, can you provide some more details please.

Soccerama commented 3 years ago

Not going to be able to answer all questions tonight as it's getting late here. My motivation was to write a function that included all of the available fields (timeInForce, minFillSize, betTargetType, betTargetSize for limitOrder) and which provided greater flexibility as using less function calls makes code quicker. This is important for many punters as kickoff nears. Yes, there are some typos and insts should be instructionList. And backwards compatability is an issue - which is why I've given it a different name. I'll have another look at this in the morning and answer your questions properly then.

Soccerama commented 3 years ago

The motivation was partly adding the extra fields, but also I found it difficult to understand why parts of the code in the existing function are there. I kept tidying things up until I got to this point but as you say, it is not backwards compatible. There is another way to get everything that I have here and maintain backwards compatibility and it was an intermediate stage in getting me to this point. It essentially takes all of the input fields then builds the same data frame structure inside the function. I will put go back to that code and put it up tomorrow.

phillc73 commented 3 years ago

I think I'd prefer the approach you suggest - building the dataframe inside the function. However, I'd also be pleased to run some benchmark speed tests against both approaches. If building the dataframe within the function is significantly slower, then there's reason to follow the alternative approach, or at least provide options for each. Happy to setup the benchmarks when there are two working functions.

Also, if there's something you don't understand in some of the code, let me know and I will try to help (keeping in mind I wrote most of this five years ago, so it could be my memory fades as well).

Soccerama commented 3 years ago

Okay, I've reworked this and it's in my fork. It uses a similar approach to the existing functions but the build of the instruction is slightly different. It should accept all documented fields but I haven't tested all of the obscure ones. It allows multiple bet types on a single race which I have tested and works. By including all fields as columns, they all have to be required or defaulted in the function definition and this means a bit of fiddling around with the df build but this doesn't cause any problems. I don't think this approach will be any slower than the alternative as the main issue is not having to delay waiting for the previous order to come back and this is achieved by allowing multiple bet types. For a dataframe of this size the time taken to build the dataframe should be negligible.

Soccerama commented 3 years ago

I've removed all of the junk from my fork and Rstudio to fix the problems from the other day. There is now a clean, documented version of a backwards compatible placeOrders function which can place all three order types with a single call and accepts all of the fields available in the Betfair API operation. It has undergone some testing and it works but I haven't really tried to break it so very happy to hear your feedback. It is in the Soccerama fork in the placeOrders_allFields branch.

phillc73 commented 3 years ago

@Soccerama Job done! Thanks a lot for all the effort you put into this. Much appreciated.

I'm closing this issue now that this commit is merged: https://github.com/phillc73/abettor/commit/df410174bcfb8ab6dfe1b7a94f334a2e3a4891e6