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

Add python wrappers and tests for new update_by operators #3731

Merged
merged 11 commits into from
Apr 28, 2023

Conversation

lbooker42
Copy link
Contributor

Operators wrapped:

  • Delta
  • RollingWAvg
  • Exponential Moving Sum
  • Exponential Min/Max
  • RollingCount
  • RollingStd

When merged, will close #3721

py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
@@ -164,6 +171,211 @@ def ema_time_decay(ts_col: str, time_scale: Union[int, str], cols: Union[str, Li
raise DHError(e, "failed to create a time-decay EMA UpdateByOperation.") from e


def ems_tick_decay(time_scale_ticks: int, cols: Union[str, List[str]],
Copy link
Member

Choose a reason for hiding this comment

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

These new methods suffer from the same problem as the ticket I have reported twice. The timescalee should not be int. It should be float.

I wasn't able to find the other ticket to link to. Maybe it was fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the already reported issues:
#2660
#3670

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is being addressed in #3745, to merge after this.

py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
py/server/deephaven/updateby.py Show resolved Hide resolved
py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
return UpdateByOperation(j_updateby_op=_JUpdateByOperation.RollingWAvg(ts_col, rev_time, fwd_time, weight_col, *cols))
except Exception as e:
raise DHError(e, "failed to create a rolling weighted average (time) UpdateByOperation.") from e
Copy link
Member

Choose a reason for hiding this comment

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

Did the new EMA std and such not make this cut?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EM Std is not going to make the EOM release. There are some complexities in the operator that need managed before it can merge.

py/server/deephaven/updateby.py Outdated Show resolved Hide resolved
@lbooker42
Copy link
Contributor Author

Added a commit that removes the _decay suffix from the python operator wrapping. There will need to be some followup in documentation since merging this would be a breaking change.

Copy link
Member

@chipkent chipkent left a comment

Choose a reason for hiding this comment

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

I'm ok with what is here, other than the open question about the default value.

… option, improved test suite, fixed an interface bug.
@lbooker42 lbooker42 merged commit 9c840c9 into deephaven:main Apr 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2023
@deephaven-internal
Copy link
Contributor

@lbooker42 lbooker42 deleted the lab-python-wrap branch June 26, 2024 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python wrappers for new UpdateBy operators
4 participants