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 note to prev expression return type for locf() #3486

Open
wants to merge 7 commits into
base: latest
Choose a base branch
from

Conversation

cracksalad
Copy link

Description

After using interpolate() for years now, I used locf() for the first time and came across a difference in their prev parameters, precisely in their return types: While interpolate() expects a tuple like (time, value), locf() expects just a value. I thought that should also be noted in the docs (as is for interpolate() but not for locf()).

Links

No issue link, but link to current locf() doc: https://docs.timescale.com/api/latest/hyperfunctions/gapfilling/time_bucket_gapfill/#locf

Review checklists

Reviewers: use this section to ensure you have checked everything before approving this PR:

Subject matter expert (SME) review checklist

  • Is the content technically accurate?
  • Is the content complete?
  • Is the content presented in a logical order?
  • Does the content use appropriate names for features and products?
  • Does the content provide relevant links to further information?

Documentation team review checklist

  • Is the content free from typos?
  • Does the content use plain English?
  • Does the content contain clear sections for concepts, tasks, and references?
  • Have any images been uploaded to the correct location, and are resolvable?
  • If the page index was updated, are redirects required
    and have they been implemented?
  • Have you checked the built version of this content?

@mkindahl
Copy link
Contributor

I noted that we lack examples for using the prev argument for both locf and interpolate so it might be a good idea to add such examples and avoid confusion.

@cracksalad
Copy link
Author

I noted that we lack examples for using the prev argument for both locf and interpolate so it might be a good idea to add such examples and avoid confusion.

Sounds like a good idea! I am willing to do that. Should I add another PR since this one is already approved?

@cracksalad
Copy link
Author

Just added the two examples here. Could still move that to another PR, if you prefer.

@mkindahl
Copy link
Contributor

I noted that we lack examples for using the prev argument for both locf and interpolate so it might be a good idea to add such examples and avoid confusion.

Sounds like a good idea! I am willing to do that. Should I add another PR since this one is already approved?

I have approved the text that you submitted, so you might want to submit this as a separate PR.

@mkindahl
Copy link
Contributor

Just added the two examples here. Could still move that to another PR, if you prefer.

Just noted. Will check them.

@mkindahl mkindahl self-requested a review October 14, 2024 06:14
Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

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

LGTM, but I suggest some minor rewrites:

  • "that is" and "for example" give a better flow of the text than "i.e." and "e.g."
  • Would be nice with an elaboration, even if informal, why time is needed in one place and not the other. I added a suggestion, but feel free to modify it.
  • The queries are using an exclusive range condition, so just checking that this was intentional and not a mistake.

api/_hyperfunctions/time_bucket_gapfill/examples.md Outdated Show resolved Hide resolved
api/_hyperfunctions/time_bucket_gapfill/examples.md Outdated Show resolved Hide resolved
cracksalad and others added 2 commits October 14, 2024 22:41
Co-authored-by: Mats Kindahl <[email protected]>
Signed-off-by: cracksalad <[email protected]>
Co-authored-by: Mats Kindahl <[email protected]>
Signed-off-by: cracksalad <[email protected]>
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