#510 #511
#510 #511
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #511 +/- ##
=======================================
Coverage 80.60% 80.60%
=======================================
Files 80 80
Lines 4732 4733 +1
Branches 809 809
=======================================
+ Hits 3814 3815 +1
Misses 641 641
Partials 277 277
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@jbogaardt can you review this PR? |
I thought I did. Did you not see my comments? Pandas is deprecating "Y" and moving towards "A" for 2.2 and later. Refer to their release note. It looks like you're code change says to use the deprecated "Y" value for newer versions of pandas which seems wrong. There is also a new conditional which doesn't actually compare against a version number: "Y" if version.Version(pd.__version__) else "A"Everything is "truthy" in python so this will always return "Y" regardless of the pandas version. |
|
I probably missed it. Sorry! Looks like I made a mistake and did the opposite. Also, there are some codecov issues, do you think we need new tests for these couple of new lines? I actually don't know how to handle it when we need to use different versions of packages. |
| grain = "Y" if float('.'.join(pd.__version__.split('.')[:-1])) < 2.2 else "A" | ||
| grain = ( | ||
| "Y" | ||
| if version.Version(pd.__version__) >= version.Version("2.2.0") |
There was a problem hiding this comment.
Should this be <= not >=?
chainladder/core/triangle.py
Outdated
| self.origin_grain, self.origin_grain | ||
| ) | ||
| freq = { | ||
| "Y": "Y" if version.Version(pd.__version__) else "A", |
There was a problem hiding this comment.
Should the conditional be completed?
|
I never saw those comments, but thanks for the feedback and approving the PR. :) I think I'll keep the equal sign the way it is in the PR, since |
|
@jbogaardt, can you double-check that In fact, I am still getting the warning: FutureWarning: 'A-DEC' is deprecated and will be removed in a future version, please use 'Y-DEC' instead. |
Thanks @kennethshsu , you're right. My brain is backwards. 'A' is being deprecated in favor of 'Y'. |
Addressed #510