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

BUG: sim._check_volume #18

Closed
sadimoodi opened this issue Jan 12, 2022 · 5 comments
Closed

BUG: sim._check_volume #18

sadimoodi opened this issue Jan 12, 2022 · 5 comments

Comments

@sadimoodi
Copy link

hello @AminHP ,
There is a bug in the check volume function inside the simulator, you write :

 def _check_volume(self, symbol: str, volume: float) -> None:
        symbol_info = self.symbols_info[symbol]
        if not (symbol_info.volume_min <= volume <= symbol_info.volume_max):
            raise ValueError(
                f"'volume' must be in range [{symbol_info.volume_min}, {symbol_info.volume_max}]"
            )
        if not round(volume / symbol_info.volume_step, 6).is_integer():
            raise ValueError(f"'volume' must be a multiple of {symbol_info.volume_step}")

you are rounding the volume to 6 decimals and expecting an integer as a result, this can only be true if the passed volume was an integer, your error message says mutiple of 0.01 (volume step), this also contradicts with your voume check function inside env._get_modified_volume where you expect volume to be mutiple of 0.01 as well.

@AminHP
Copy link
Owner

AminHP commented Jan 12, 2022

Hi @sadimoodi ,

I don't see the problem.

>>> round(2.545 / 0.01, 6).is_integer()
False
>>> round(2.54 / 0.01, 6).is_integer()
True

@sadimoodi
Copy link
Author

then why not just use:

v =  round(v ,2)

This will ensure that the result is a mutiplication of 0.01 without the need for the above validation, you are expecting a two decimal point float in _get_modified_volume because you use:

v = round(v / si.volume_step) * si.volume_step

isnt the above approach simpler and faster?

@AminHP
Copy link
Owner

AminHP commented Jan 13, 2022

Because volume_step is not always 0.01. It is variable and I wanted to support all cases.

@sadimoodi
Copy link
Author

in your pickle files all the instruments where having 0.01 as volume step but i understand where you come from.
in my case i am building a crypto trading agent which does not use contract size nor volumes to trade, rather amounts and leverage, so this step was not needed in my case, i just round to 2 decimal points (simpler)

@AminHP
Copy link
Owner

AminHP commented Jan 15, 2022

Hmm, I see. So it's not a bug. Anyway, I don't think it has a huge impact on execution speed.

@AminHP AminHP closed this as completed Jan 17, 2022
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

No branches or pull requests

2 participants