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

Differentiate duration and datetime #83

Closed
wants to merge 5 commits into from
Closed

Conversation

aricohen93
Copy link
Collaborator

@aricohen93 aricohen93 commented Jun 8, 2022

Description

Differentiate results of type duration and datetime.

Identified problem:

When applying the dates pipeline in batch, we obtain in the same column of the result dataframe objects of differnt data type.
Also, the to_datetime() method of a RelativeDate class returns either datetime or duration types depending if note_datetime is given

Some ideas:

  • save spans into different keys
  • replace to_datetime() by to_pendulum() or maybe to_datetime should be a wraper of to_duration() and to_absolute()

Checklist

  • If this PR is a bug fix, the bug is documented in the test suite.
  • Changes were documented in the changelog (pending section).
  • If necessary, changes were made to the documentation (eg new pipeline).

@aricohen93 aricohen93 requested review from percevalw and bdura June 8, 2022 15:22
@aricohen93 aricohen93 assigned aricohen93 and percevalw and unassigned aricohen93 Jun 8, 2022
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 60.52% and project coverage change: -0.26 ⚠️

Comparison is base (74fa4c7) 94.06% compared to head (35815ad) 93.80%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   94.06%   93.80%   -0.26%     
==========================================
  Files         172      172              
  Lines        5085     5103      +18     
==========================================
+ Hits         4783     4787       +4     
- Misses        302      316      +14     
Impacted Files Coverage Δ
edsnlp/pipelines/misc/dates/models.py 89.24% <59.45%> (-8.62%) ⬇️
edsnlp/pipelines/qualifiers/history/history.py 94.51% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@percevalw percevalw marked this pull request as ready for review March 17, 2023 07:48
@percevalw percevalw removed the request for review from bdura March 17, 2023 07:50
@percevalw
Copy link
Member

percevalw commented Mar 17, 2023

I've rebased on master, moved durations to a new duration span group (this one maybe a breaking change !) and added an option to assign a shared label date (except for durations)
I feel like having absolute and relative labels for dates was a bit unnecessary given their shared API. What's your opinion on this @Aremaki ?

@percevalw percevalw requested a review from Aremaki March 17, 2023 08:16
@percevalw
Copy link
Member

Closing as this was merged in #213

@percevalw percevalw closed this Sep 15, 2023
@percevalw percevalw deleted the to-duration branch November 14, 2024 20:27
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