Use numpy datetime64 instead of datetime.date #77 #84
Conversation
rlskoeser
force-pushed
the
experiment/numpy-datetime64
branch
from
81299bb
to
3306bde
Compare
rlskoeser
changed the title
rlskoeser
changed the title
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe changes involve the addition of type checking with MyPy in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Undate
participant Date
User->>Undate: Initialize with dates
Undate->>Date: Create Date instances
Date-->>Undate: Return Date objects
User->>Undate: Call duration()
Undate->>Date: Calculate duration
Date-->>Undate: Return duration value
Undate-->>User: Return duration result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range, codebase verification and nitpick comments (2)
setup.cfg (1)
44-44: Approved: Addition of NumPy as a DependencyThe inclusion of
numpyin theinstall_requiressection is crucial for the project's functionality, especially given the shift to usingnumpy.datetime64. However, consider the implications of this addition on installation size and dependency management, particularly for users in constrained environments.Consider making
numpyan optional dependency if feasible, to accommodate users who may not require the extended functionality.src/undate/undate.py (1)
Line range hint
287-316: Review thedurationmethod implementation.The
durationmethod has been significantly changed to accommodate the newDateclass and potentially returnnumpy.timedelta64. Ensure that the logic correctly calculates the duration based on the date precision and handles all edge cases, especially with unknown or partially known dates.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- pyproject.toml (1 hunks)
- setup.cfg (1 hunks)
- src/undate/date.py (1 hunks)
- src/undate/dateformat/iso8601.py (1 hunks)
- src/undate/undate.py (9 hunks)
- tests/test_date.py (1 hunks)
- tests/test_undate.py (3 hunks)
Additional context used
Ruff
tests/test_date.py
1-1:
numpyimported but unusedRemove unused import:
numpy(F401)
3-3:
undate.date.ONE_DAYimported but unusedRemove unused import
(F401)
3-3:
undate.date.ONE_YEARimported but unusedRemove unused import
(F401)
3-3:
undate.date.ONE_MONTH_MAXimported but unusedRemove unused import
(F401)
24-24: Redefinition of unused
test_init_year_monthfrom line 18(F811)
42-42: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
src/undate/date.py
4-4:
typing.Dictimported but unusedRemove unused import
(F401)
4-4:
typing.Unionimported but unusedRemove unused import
(F401)
68-68: Use
self.dtype != "datetime64[Y]"instead ofnot self.dtype == "datetime64[Y]"Replace with
!=operator(SIM201)
src/undate/undate.py
6-6:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
8-8:
numpyimported but unusedRemove unused import:
numpy(F401)
9-9:
numpy.typing.ArrayLikeimported but unusedRemove unused import
(F401)
9-9:
numpy.typing.DTypeLikeimported but unusedRemove unused import
(F401)
Additional comments not posted (3)
pyproject.toml (1)
8-9: Approved: Enhanced Type Checking for NumPyThe addition of the
[tool.mypy]section with the NumPy typing plugin is a significant improvement for the project. It ensures enhanced type safety and better integration with NumPy, which is crucial given the project's new dependency on this library.src/undate/undate.py (2)
267-267: Static method usage.The
from_datetime_datemethod is correctly updated to handle thedatetime.dateinput and convert it to anUndateobject. This method serves as a bridge between the olddatetime.dateusage and the newDateclass, facilitating easier transition and integration.
128-129: Initialization ofDateobjects.The
earliestandlatestattributes of theUndateclass are now initialized using the customDateclass. Ensure that theDateclass constructor correctly handles the extreme values and any edge cases that might arise from the broad range of years.Verification successful
Initialization of
Dateobjects is well-handled.The
Dateclass uses NumPy'sdatetime64for date handling, and test cases cover various initialization scenarios, ensuring robustness against edge cases. The__new__method inDatehandles year, month, and day parameters effectively. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the Date class constructor with extreme values. ast-grep --lang python --pattern $'class Date { $$$ __init__($_, $_, $_) { $$$ } $$$ }'Length of output: 447
Script:
#!/bin/bash # Correct the pattern to search for the Date class and its __init__ method in Python. ast-grep --lang python --pattern $'class Date:\n def __init__(self, $_, $_, $_):\n $$$'Length of output: 90
Script:
#!/bin/bash # Search for any class definitions related to dates in the codebase. rg 'class\s+\w+' -A 2 # Search for any imports related to date functionality. rg 'import\s+\w+' -A 2Length of output: 11688
Script:
#!/bin/bash # Extract the __init__ method of the Date class from src/undate/date.py to review its implementation. ast-grep --lang python --pattern $'class Date(np.ndarray):\n def __init__(self, $_, $_, $_):\n $$$'Length of output: 102
Script:
#!/bin/bash # Search for method definitions within the Date class in src/undate/date.py. rg 'def\s+\w+' src/undate/date.py -A 2 # Search for test cases that use the Date class to check for handling of extreme values. rg 'Date\(' tests/ -A 2Length of output: 1921
| assert d.dtype == "datetime64[M]" | ||
| assert str(d) == "2010-05" | ||
|
|
||
| def test_init_year_month(self): |
There was a problem hiding this comment.
Correct Redefinition of Test Function
The function test_init_year_month is defined twice (lines 18 and 24), which is likely an oversight. This redefinition could cause one of the tests to be skipped, leading to incomplete test coverage.
Consider renaming one of the functions or combining them if they are intended to test different scenarios. Here is a proposed fix to rename the second function to test_init_year_month_day:
- def test_init_year_month(self):
+ def test_init_year_month_day(self):Also applies to: 18-18
Tools
Ruff
24-24: Redefinition of unused
test_init_year_monthfrom line 18(F811)
There was a problem hiding this comment.
oh yeah, I think I found and fixed this in a different branch when I was working on it yesterday
There was a problem hiding this comment.
@rlskoeser, thanks for the update! Since you've already fixed this in another branch, no further action is needed here regarding the duplicate test function. I'll proceed with the rest of the review.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| import numpy as np | ||
|
|
||
| from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX |
There was a problem hiding this comment.
Remove Unused Imports
The static analysis has correctly identified several unused imports:
numpyis imported but not used directly in the tests.ONE_DAY,ONE_YEAR, andONE_MONTH_MAXare imported but not used.
Consider removing these to clean up the code and reduce confusion about dependencies.
Apply this diff to remove the unused imports:
-import numpy as np
-from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX
+from undate.date import Date, DatePrecisionCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import numpy as np | |
| from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX | |
| from undate.date import Date, DatePrecision |
Tools
Ruff
1-1:
numpyimported but unusedRemove unused import:
numpy(F401)
3-3:
undate.date.ONE_DAYimported but unusedRemove unused import
(F401)
3-3:
undate.date.ONE_YEARimported but unusedRemove unused import
(F401)
3-3:
undate.date.ONE_MONTH_MAXimported but unusedRemove unused import
(F401)
|
|
||
| def test_properties_day(self): | ||
| assert Date(2001).day is None | ||
| assert Date(2010, 5).day == None |
There was a problem hiding this comment.
Use is for None Comparisons
The comparison to None using == is not recommended. Using is is the correct practice in Python for clarity and correctness.
Apply this diff to correct the comparison:
- assert Date(2010, 5).day == None
+ assert Date(2010, 5).day is NoneCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert Date(2010, 5).day == None | |
| assert Date(2010, 5).day is None |
Tools
Ruff
42-42: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
| from enum import IntEnum | ||
|
|
||
| # Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None | ||
| from typing import Optional, Dict, Union |
There was a problem hiding this comment.
Remove unused imports.
The imports for Dict and Union from the typing module are not used in the file. Removing these will clean up the code and reduce confusion about dependencies.
Apply this diff to remove the unused imports:
-from typing import Optional, Dict, Union
+from typing import OptionalCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from enum import IntEnum | |
| # Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None | |
| from typing import Optional, Dict, Union | |
| from enum import IntEnum | |
| # Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None | |
| from typing import Optional |
Tools
Ruff
4-4:
typing.Dictimported but unusedRemove unused import
(F401)
4-4:
typing.Unionimported but unusedRemove unused import
(F401)
| @property | ||
| def year(self): | ||
| return int(str(self.astype("datetime64[Y]"))) | ||
|
|
||
| @property | ||
| def month(self): | ||
| # if date unit is year, don't return a month (only M/D) | ||
| if not self.dtype == "datetime64[Y]": | ||
| return int(str(self.astype("datetime64[M]")).split("-")[-1]) | ||
|
|
||
| @property | ||
| def day(self): | ||
| # only return a day if date unit is in days | ||
| if self.dtype == "datetime64[D]": | ||
| return int(str(self.astype("datetime64[D]")).split("-")[-1]) |
There was a problem hiding this comment.
Refine logical expressions in property methods.
The properties month and day use a less readable form of checking the data type. Replacing not self.dtype == "datetime64[Y]" with self.dtype != "datetime64[Y]" will improve the readability and maintainability of the code.
Apply this diff to refine the logical expression:
- if not self.dtype == "datetime64[Y]":
+ if self.dtype != "datetime64[Y]":Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def year(self): | |
| return int(str(self.astype("datetime64[Y]"))) | |
| @property | |
| def month(self): | |
| # if date unit is year, don't return a month (only M/D) | |
| if not self.dtype == "datetime64[Y]": | |
| return int(str(self.astype("datetime64[M]")).split("-")[-1]) | |
| @property | |
| def day(self): | |
| # only return a day if date unit is in days | |
| if self.dtype == "datetime64[D]": | |
| return int(str(self.astype("datetime64[D]")).split("-")[-1]) | |
| @property | |
| def year(self): | |
| return int(str(self.astype("datetime64[Y]"))) | |
| @property | |
| def month(self): | |
| # if date unit is year, don't return a month (only M/D) | |
| if self.dtype != "datetime64[Y]": | |
| return int(str(self.astype("datetime64[M]")).split("-")[-1]) | |
| @property | |
| def day(self): | |
| # only return a day if date unit is in days | |
| if self.dtype == "datetime64[D]": | |
| return int(str(self.astype("datetime64[D]")).split("-")[-1]) |
Tools
Ruff
68-68: Use
self.dtype != "datetime64[Y]"instead ofnot self.dtype == "datetime64[Y]"Replace with
!=operator(SIM201)
| return all( | ||
| [ | ||
| self.earliest <= other.earliest, | ||
| self.latest >= other.latest, | ||
| # is precision sufficient for comparing partially known dates? | ||
| self.precision > other.precision, | ||
| ] |
There was a problem hiding this comment.
Review precision comparison in __contains__ method.
The comparison of self.precision > other.precision in the __contains__ method assumes a certain hierarchy or ordering of precision values. This logic might not be intuitive or could lead to incorrect results if the precision levels are not strictly comparable in a linear fashion.
|
|
||
| # Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None | ||
| from typing import Optional, Dict, Union | ||
| from typing import Optional, Dict, Union, Any |
There was a problem hiding this comment.
Remove unused import.
The import for typing.Any is flagged as unused. Removing unused imports helps maintain the cleanliness and readability of the code.
Proposed removal:
-from typing import Optional, Dict, Union, Any
+from typing import Optional, Dict, UnionCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import Optional, Dict, Union, Any | |
| from typing import Optional, Dict, Union |
Tools
Ruff
6-6:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
| import numpy as np | ||
| from numpy.typing import ArrayLike, DTypeLike |
There was a problem hiding this comment.
Remove unused imports.
The imports for numpy, ArrayLike, and DTypeLike are flagged as unused by static analysis tools. If these are not used in the current implementation, they should be removed to clean up the code.
Proposed removal:
-import numpy as np
-from numpy.typing import ArrayLike, DTypeLikeCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import numpy as np | |
| from numpy.typing import ArrayLike, DTypeLike |
Tools
Ruff
8-8:
numpyimported but unusedRemove unused import:
numpy(F401)
9-9:
numpy.typing.ArrayLikeimported but unusedRemove unused import
(F401)
9-9:
numpy.typing.DTypeLikeimported but unusedRemove unused import
(F401)
| elif date_portion == "month": | ||
| date_parts.append("%02d" % undate.earliest.month) | ||
| elif date_portion == "day": | ||
| date_parts.append("%02d" % undate.earliest.day) | ||
|
|
There was a problem hiding this comment.
Enhanced date portion handling in to_string method.
The modifications to explicitly handle "month" and "day" portions improve the method's ability to format dates accurately. The use of string formatting ensures that these portions are always represented with two digits, which enhances consistency.
However, the removal of the previously commented-out code (lines 69-74) reduces the method's flexibility to handle other date portions dynamically. If flexibility in handling various date formats is a requirement, consider reintroducing or refactoring this part of the code.
| # assert isinstance(day_duration, timedelta) | ||
| assert day_duration.astype("int") == 1 | ||
|
|
||
| january_duration = Undate(2022, 1).duration() | ||
| assert january_duration.days == 31 | ||
| assert january_duration.astype("int") == 31 | ||
| feb_duration = Undate(2022, 2).duration() | ||
| assert feb_duration.days == 28 | ||
| assert feb_duration.astype("int") == 28 | ||
| # next leap year will be 2024 | ||
| leapyear_feb_duration = Undate(2024, 2).duration() | ||
| assert leapyear_feb_duration.days == 29 | ||
| assert leapyear_feb_duration.astype("int") == 29 | ||
|
|
||
| year_duration = Undate(2022).duration() | ||
| assert year_duration.days == 365 | ||
| assert year_duration.astype("int") == 365 | ||
| leapyear_duration = Undate(2024).duration() | ||
| assert leapyear_duration.days == 366 | ||
| assert leapyear_duration.astype("int") == 366 |
There was a problem hiding this comment.
Updated assertions in test_duration method.
The replacement of timedelta assertions with astype("int") reflects a change in the duration method's return type. This update simplifies the assertions and aligns them with the new method implementation.
However, it would be beneficial to add comments or documentation in the Undate class to clarify why the duration method's return type was changed and how it should be used.
Would you like me to help draft the necessary documentation or comments?
|
Observations on
|
| # adapted from https://stackoverflow.com/a/27129510/9706217 | ||
|
|
||
| def __new__(cls, year: int, month: Optional[int] = None, day: Optional[int] = None): | ||
| if isinstance(year, np.datetime64): |
There was a problem hiding this comment.
Maybe it's my non-existing knowledge about typing in python, but shouldn't year be of type int per line 24? Or is datetime64 a subclass of int?
There was a problem hiding this comment.
I guess my question is if the type of year should be a different one :)
There was a problem hiding this comment.
Most of my comments are in setup.cfg around NumPy - mostly just thinking through the implications of adding it as a dependency. Overall I think I am sold as it expands the capability of the undate package quite a bit. It would be useful to add some benchmarks to the release notes for the next version to detail the previous and new installed sizes for undate, and the previous / new runtimes for some operations to see how numpy impacts performance and payload.
| assert d.dtype == "datetime64[M]" | ||
| assert str(d) == "2010-05" | ||
|
|
||
| def test_init_year_month(self): |
| # numpy datetime is stored as 64-bit integer, so min/max | ||
| # depends on the time unit; assume days for now | ||
| # See https://numpy.org/doc/stable/reference/arrays.datetime.html#datetime-units | ||
| max_year = int(2.5e16) | ||
| min_year = int(-2.5e16) |
There was a problem hiding this comment.
I don't know that I agree with the suggestion that this be configurable or what the advantages of that would be, if we're initializing a 64bit int I don't think making the min/max smaller would save any memory. If we're committing to NumPy then we should use it consistently.
| @@ -41,6 +41,7 @@ python_requires = >=3.8 | |||
| install_requires = | |||
| python-dateutil | |||
| lark | |||
| numpy | |||
There was a problem hiding this comment.
If we merge #88 first, we'll have to move this to the pyproject.toml dependencies.
There was a problem hiding this comment.
Do we want to limit this to NumPy v2, which was released in May? That was the first major release since 2006. If we do limit it to v2, we should also drop support for Python < 3.10. It looks like NumPy v2 requires Python >= 3.10.
I'm pretty sure that there are no direct Python dependencies, just some optional dependencies for speeding up operations. pip should provide pre-built wheels.
NumPy looks like it will add 30-80MB to the payload. For most applications that's not an issue. But maybe we should benchmark the current version of undate and the size with numpy? The environments that I'm typically space-constrained in are Lambdas. I think there is still a 50MB (zipped) and 250MB (unzipped) limit for Lambda functions, so this could take up a significant chunk of that.
There was a problem hiding this comment.
I think I'm still leaning towards using NumPy given the need to support dates outside of 9999. The other way that we could go is with struct_time, which is what python-edtf uses. But it's a little clunky and you'd still have to have a shim class (like you add here) or a conversion function.
There was a problem hiding this comment.
Trying to make NumPy optional seems like a nightmare
There was a problem hiding this comment.
I was thinking it would be nice to move to Python 3.10+ anyway, it gets us improved python type hinting and 3.9 is getting old enough at this point that it seems reasonable to drop.
|
I'll update this and reconcile with the other merged PRs, address the flagged comments about type hints, and then do some testing and documentation of benchmarking as suggested before I merge this. |
![kypso-io[bot]](https://avatars.githubusercontent.com/in/843074?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
- Number of files reviewed: 7
- Number of comments: 3
- Number of suggestions: 0
📚 File changes
| File path | File changes |
|---|---|
| pyproject.toml | Added configuration for mypy to include numpy typing plugin. |
| setup.cfg | Added numpy as a required dependency in the install_requires section. |
| src/undate/date.py | Introduced a custom Date class for enhanced date handling with properties for year, month, and day. |
| src/undate/dateformat/iso8601.py | Refactored the date formatting logic to directly append formatted strings for year, month, and day instead of using strftime. |
| src/undate/undate.py | No summary. |
| tests/test_date.py | Added tests for the new Date and DatePrecision classes. |
| tests/test_undate.py | Removed the DatePrecision class and its associated tests, and updated duration tests to use astype('int') instead of checking for timedelta. |
| raise Exception( | ||
| f"Unable to parse dates adequately as {expected_dtype}: {data}" | ||
| ) |
There was a problem hiding this comment.
The exception raised in the __new__ method does not specify the type of error. It would be better to raise a more specific exception, such as ValueError, to indicate that the input date could not be parsed correctly.
| # if date unit is year, don't return a month (only M/D) | ||
| if not self.dtype == "datetime64[Y]": | ||
| return int(str(self.astype("datetime64[M]")).split("-")[-1]) | ||
|
|
There was a problem hiding this comment.
The month property does not have a return statement if the condition is not met. This could lead to returning None implicitly, which may not be the intended behavior. Consider raising an exception or returning a default value instead.
| def day(self): | ||
| # only return a day if date unit is in days | ||
| if self.dtype == "datetime64[D]": | ||
| return int(str(self.astype("datetime64[D]")).split("-")[-1]) |
There was a problem hiding this comment.
The day property also lacks a return statement if the condition is not met. Similar to the month property, consider raising an exception or returning a default value to avoid returning None implicitly.
ref #77
datetime.dateprevious notes
I implemented this as spike to see how hard it would be to use numpy's datetime64 instead of python's builtin datetime.date. Using
np.datetime64gets us way past the year limitation ofdatetime.date([2.5e16 BC, 2.5e16 AD] for day-precision dates).We can make this an optional dependency so you would only install numpy if you need the expanded year range. Making it optional makes the code and the testing more complicated. Do you think numpy as a dependency would be a concern for folks doing e.g web-based projects?
(type check is failing because I haven't bothered updating the types yet)
Summary by CodeRabbit
New Features
Dateclass for enhanced date handling with properties for year, month, and day.DatePrecisionenumeration for categorizing date precision levels.numpyas a required dependency to support numerical computations.Bug Fixes
Tests
DateandDatePrecisionclasses to ensure functionality.Undateclass to reflect changes in duration handling.