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

[CODE WIDE] Operator chaining #206

Merged
merged 28 commits into from
Jul 24, 2023
Merged

[CODE WIDE] Operator chaining #206

merged 28 commits into from
Jul 24, 2023

Conversation

ianspektor
Copy link
Collaborator

@ianspektor ianspektor commented Jul 19, 2023

TODO

  • try inherited_members or some similar hack
  • update notebooks after Eager notebooks #202 is merged
  • migrate window ops
  • migrate calendar ops

✅ good:

  • loving the syntax already
  • autocomplete shows output types correctly
image image

⚠️ not good:

  • docstrings need to be defined in the EventSetOperationsMixin class.
    • functools.wraps doesn't work because it makes autocomplete think the function is expecting the evset as first argument, but it isn't, since it's a class method that receives self. also doesn't show up in docs.
    • assigning the function to the class (either during definition, in the constructor, or dynamically after creating it) doesn't work because it gets marked as a variable. also it doesn't show up in docs.

@ianspektor ianspektor requested a review from achoum July 19, 2023 16:32
@@ -96,6 +96,7 @@ plugins:
group_by_category: true
show_category_heading: false
show_root_heading: true
inherited_members: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did it worked?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It did :) the option literally got released last week in https://github.com/mkdocstrings/python/releases/tag/1.2.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's what it's called being lucky :)

@ianspektor
Copy link
Collaborator Author

Opened issue to mkdocstrings-python to fix full EventSet source path being shown in docs mkdocstrings/python#90

@ianspektor ianspektor changed the title first attempt at chaining [CODE WIDE] Operator chaining Jul 21, 2023
@ianspektor
Copy link
Collaborator Author

Opened another issue for weird behavior when linking to temporian.EventSet.xxx methods mkdocstrings/python#91

Copy link
Collaborator

@achoum achoum left a comment

Choose a reason for hiding this comment

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

This is amazing. Thanks for pulling this off!!

Nit + optional (another pr): Can you run "python tools/build_cleaner.py" to see if deps need to be updated.

@@ -96,6 +96,7 @@ plugins:
group_by_category: true
show_category_heading: false
show_root_heading: true
inherited_members: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's what it's called being lucky :)

temporian/core/operators/binary/arithmetic.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@DonBraulio DonBraulio left a comment

Choose a reason for hiding this comment

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

Go ahead and merge this one and I can update the notebooks in #202 altogether. That one is ready but not approved yet, so let's resolve this one first if it's ready now.

@ianspektor ianspektor merged commit e3aab00 into main Jul 24, 2023
@ianspektor ianspektor deleted the op-chaining branch July 24, 2023 17:53
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.

3 participants