Use uncertainties.ufloat for uncertain durations #116
Conversation
WalkthroughThe pull request updates the project to support uncertainty in date calculations. The changes add the Changes
Sequence Diagram(s)sequenceDiagram
participant U as Undate Instance
participant D as duration() Method
participant C as Calculation Logic
participant R as Return Value
U->>D: Call duration()
D->>C: Evaluate date precision and unknown year conditions
alt Multiple possible month durations
C-->>D: Provide list of possible day counts
D->>R: Create and return Udelta (uncertain duration)
else Single valid duration
C-->>D: Calculate exact day count
D->>R: Create and return Timedelta (certain duration)
end
D-->>U: Return computed duration
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #116 +/- ##
===========================================
+ Coverage 96.50% 97.27% +0.76%
===========================================
Files 20 21 +1
Lines 658 697 +39
===========================================
+ Hits 635 678 +43
+ Misses 23 19 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
pyproject.toml (1)
12-12: Appropriate dependency addition for new functionality.Adding the
uncertaintiespackage as a dependency is necessary for the implementation ofufloatfor uncertain durations. Consider specifying a minimum version to ensure compatibility with your implementation.-dependencies = ["lark[interegular]", "numpy", "convertdate", "strenum; python_version < '3.11'", "uncertainties"] +dependencies = ["lark[interegular]", "numpy", "convertdate", "strenum; python_version < '3.11'", "uncertainties>=3.1.7"]src/undate/date.py (1)
34-53: Well-implemented uncertain timedelta class.The
Udeltaclass is a good implementation of an uncertain duration. The use of@dataclassis appropriate, and the calculation of midpoint and half-difference is mathematically sound for representing uncertainty.A few suggestions for potential improvements:
- Consider adding more timedelta-like functionality as noted in your TODO comment
- Add validation to ensure that at least two day values are provided
- Consider implementing comparison methods (
__eq__,__lt__, etc.)@dataclass class Udelta: """An uncertain timedelta, for durations where the number of days is uncertain. Initialize with a list of possible day durations as integers, which are used to calculate a value for duration in :attr:`days` as an instance of :class:`uncertainties.ufloat`. """ # NOTE: we will probably need other timedelta-like logic here besides days... #: number of days, as an instance of :class:`uncertainties.ufloat` days: ufloat def __init__(self, *days: int): + if len(days) < 2: + raise ValueError("At least two possible day values are required to create uncertainty") min_days = min(days) max_days = max(days) half_diff = (max_days - min_days) / 2 midpoint = min_days + half_diff self.days = ufloat(midpoint, half_diff) + + def __eq__(self, other): + if isinstance(other, Udelta): + return self.days == other.days + return NotImplementedsrc/undate/converters/calendars/gregorian.py (1)
43-44: Consider implementing uncertainty for maximum days.The TODO comment raises a valid question about returning a
ufloatfor the maximum possible days when month and year are unknown. This would be consistent with the newUdeltaapproach.Consider implementing this enhancement to return a
ufloatwith uncertainty range for the maximum day when month/year are unknown:# if year and month are unknown, return maximum possible # TODO: should this return a ufloat? -max_day = 31 +from uncertainties import ufloat +# Create ufloat with average of 28-31 days and half-difference of (31-28)/2 +max_day = ufloat(29.5, 1.5)tests/test_date.py (1)
84-94: Add broader boundary tests
The newly introducedTestUdeltaclass tests uncertain durations for a limited scenario (28–29 days). Consider adding edge cases (e.g., no possible days, a single possible day) to verify more exotic usage or error handling.src/undate/undate.py (3)
441-470: Partial unknown year logic
This block correctly attempts leap and non-leap year combinations when the year is unknown. However, consider extending support for partially known years (e.g.,19XX) and non-Gregorian calendars.Would you like me to open an issue to track more comprehensive handling for partially known years or other calendar systems?
472-474: Potential empty set risk
Ifpossible_max_daysis ever empty, accessing[0]will raise anIndexError. Confirm thatpossible_max_daysalways has at least one element for these cases.- max_day = list(possible_max_days)[0] + if not possible_max_days: + # handle gracefully, e.g. return ONE_DAY or raise an error + max_day = list(possible_max_days)[0]
481-482:FIXMEcomment on calendar dependence
A flat 31-day maximum may not be correct across all calendars.Would you like me to open a new issue or propose a patch to account for different calendar logic (e.g., Hijri, Hebrew) when determining the maximum month length?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pyproject.toml(1 hunks)src/undate/converters/calendars/gregorian.py(2 hunks)src/undate/date.py(2 hunks)src/undate/interval.py(1 hunks)src/undate/undate.py(3 hunks)tests/test_date.py(2 hunks)tests/test_undate.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/test_undate.py (1)
src/undate/date.py (3)
Timedelta(11-31)Udelta(35-52)days(29-31)
tests/test_date.py (1)
src/undate/date.py (5)
Date(63-142)DatePrecision(145-170)Timedelta(11-31)Udelta(35-52)days(29-31)
🔇 Additional comments (12)
src/undate/interval.py (1)
107-111: Well-documented future enhancement plan.This comment correctly identifies a case where uncertain durations should be considered - when intervals span the end of February in Gregorian calendars. The implementation of the
Udeltaclass in other files enables this future enhancement.src/undate/date.py (1)
2-2: Good import choices for the new functionality.The imports for
dataclassesanduncertainties.ufloatare appropriate for implementing the new uncertain duration class.Also applies to: 8-8
src/undate/converters/calendars/gregorian.py (1)
16-19: Good documentation improvement for leap year constants.Clarifying that these are "arbitrary" known leap/non-leap years improves the code documentation. The addition of
LEAP_YEARwill be useful for calculations involving February 29.tests/test_date.py (2)
2-2: New import forufloat
This import is necessary for testing the new uncertain duration functionality.
4-4: AddedUdeltaimport
The import aligns with the new uncertain timedelta tests.src/undate/undate.py (3)
21-21: Import statement forUdelta
This import is consistent with returning eitherTimedeltaorUdeltain thedurationmethod.
423-423: Union return type forduration
UsingTimedelta | Udeltarequires Python 3.10+ or use offrom __future__ import annotations. Confirm that your environment meets these requirements, or adjust if Python < 3.10 is needed.
429-430: Updated docstring
Accurately reflects the new behavior returningUdeltafor uncertain durations.tests/test_undate.py (4)
7-7: New import ofUdelta
The test suite now checks uncertain durations. This import is appropriate.
386-392: Check partial unknown month (“1X”)
Returning aUdeltafor typical end-of-year months (October–December) is correctly validated.
393-399: Test unknown month (“XX”)
Validates a 28–31 day range for unknown months by returning aUdeltawith a midpoint and deviation. Looks good.
400-406: Uncertain February duration
Correctly handles the 28–29 day range for an unknown year. This test accurately ensures theUdeltabehavior.
|
Mostly looking at the test suite to see how the implementation works, rather than the internals since I don't have a great understanding of the entire code structure. Mostly this works great. However, the one concern I have is that that |
|
@taylor-arnold thank you for looking and testing! I hadn't gotten that far with trying out how Any thoughts on how to proceed? Would it be better to create a simple uncertain range (or set?) class here? I did wonder about the fact that in this case we know it's more likely to be 28 than 29 - but I'm not sure how much that matters. If you have capacity to propose expected behavior for some of these uncertain duration calculations I could start building out unit tests and use that to drive the implementation. |
|
Yes, I definitely agree that it's great to not rebuild the wheel if possible. Though, it's probably not too much of a push to make a minimum object type that records minimum and maximum and allows for them to be added/substracted would not be too difficult. There's also this package that seems like it does just that. Could use their structures, or if it's too heavy-weight, maybe just incorporate the examples as test cases? https://github.com/AlexandreDecan/portion |
|
@taylor-arnold that library looks interesting! Only one additional dependency and has potential applications for other use cases — EDTF supports sets of dates and I hadn't yet wrapped my head around how one might implement or use that. I'll revisit this swapping out uncertainties for portions and see how it goes. Appreciate the feedback & suggestion. |
|
@all-contributors please add @taylor-arnold for review and ideas |
|
I've put up a pull request to add @taylor-arnold! 🎉 |
preliminary implementation to add support for uncertain values
ufloatobjectUdeltaobject for a timedelta with uncertain duration, which usesufloatfor daysUndate.durationto calculate and returnUdeltawhen there is uncertainty about the durationSummary by CodeRabbit
New Features
Chores