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

fix / avellaneda status msg #3195

Merged
merged 3 commits into from Apr 19, 2021
Merged

Conversation

keithbaum
Copy link
Contributor

@keithbaum keithbaum commented Apr 9, 2021

Changed condition to check for "volatility is not NaN" since when volatility=0 although being valid number "bool(volatility) =False" and status message was not showing.

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:

@keithbaum keithbaum changed the title Fix avellaneda status msg fix / avellaneda status msg Apr 9, 2021
@keithbaum keithbaum linked an issue Apr 9, 2021 that may be closed by this pull request
@RC-13
Copy link
Contributor

RC-13 commented Apr 9, 2021

Tested and confirmed that PR fix the reported issue.
Liquid:
image
Kraken:
image
Observe the bots for 1 hour and the strategy parameters doesn't go away

RC-13
RC-13 previously approved these changes Apr 9, 2021
@keithbaum keithbaum self-assigned this Apr 9, 2021
Copy link
Contributor

@CrimsonJacket CrimsonJacket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. I do know that checking if a variable is NaN, we can't just use a is not NaN or a != NaN. We should use math.isnan() or its equivalent.

image

Copy link
Contributor

@PtrckM PtrckM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-test no issue found (run time 1hr), reapproving
image

Copy link
Contributor

@CrimsonJacket CrimsonJacket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dennisocana dennisocana merged commit cb583ad into development Apr 19, 2021
@dennisocana dennisocana deleted the fix/avellaneda_status_msg branch April 19, 2021 05:04
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.

Avellaneda MM - Strategy parameters not showing on status output
5 participants