search

casact / chainladder-python

Actuarial reserving in Python
https://chainladder-python.readthedocs.io/en/latest/
Mozilla Public License 2.0
191 stars 71 forks source link

#510 #511

Closed kennethshsu closed 6 months ago

kennethshsu commented 6 months ago

Addressed #510

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 41.66667% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 80.60%. Comparing base (71a6ced) to head (f30f353).

Files Patch % Lines
chainladder/core/correlation.py 22.22% 14 Missing :warning:
Additional details and impacted files ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/casact/chainladder-python/pull/511/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=casact) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/casact/chainladder-python/pull/511/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=casact) | `80.60% <41.66%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=casact#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kennethshsu commented 6 months ago

@jbogaardt can you review this PR?

jbogaardt commented 6 months ago

@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.

kennethshsu commented 6 months ago

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.

kennethshsu commented 6 months ago

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 2.2 and later means >= 2.2.

kennethshsu commented 6 months ago

@jbogaardt, can you double-check that A is preferred and that Y is being deprecated by pandas? I just checked the doc you linked and I don't see it.

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.
jbogaardt commented 6 months ago

@jbogaardt, can you double-check that A is preferred and that Y is being deprecated by pandas? I just checked the doc you linked and I don't see it.

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'.