#514

[kennethshsu]

• Addressed Development Issue #514.
  This is done by removing secondary_rank.
  @henrydingliu, do you remember why you did this? None of the tests failed, and I couldn't figure out what its supposed to do.
• Added test in Bugs in drop_high and drop_low #517.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.58%. Comparing base (d21f125) to head (1bc0567).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #519   +/-   ##
=======================================
  Coverage   80.57%   80.58%           
=======================================
  Files          80       80           
  Lines        4756     4758    +2     
  Branches      810      810           
=======================================
+ Hits         3832     3834    +2     
  Misses        648      648           
  Partials      276      276           

Flag	Coverage Δ
unittests	80.58% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kennethshsu]

If this PR is approved, I think I will need to go back and remove all the dependent sub functions to not pass in that argument. We will need to modify the tests too. I'm sure Henry has a reason for this, but it's also been a while. I wonder if it was a way to get around some upstream bug.

[henrydingliu]

someone had an issue where drop_hilo behaved unpredictably. on a column like this

1.2
1
1
1
1.1
1

if you do ex hilo and volume weighted, it wasn't obvious which 1 would be dropped. secondary rank would allow you to specify if you want to drop maybe the oldest 1 or the 1 with the smallest underlying loss.

I'm surprise that removing secondary_rank would do anything. it looked to be an optional argument that defaults to blank.

[kennethshsu]

I see, that makes a lot of sense now after reading the comments again. But this is not currently captured in any of the tests right? That's why this PR is still passing all tests.

Removing the argument does fix #514 because these lines are causing problems. The primary is obj.age_to_age but the secondary is obj.iloc[...,:-1,:-1]. However, I believe we can only do this when the triangle is symmetrical (i.e. an isosceles right triangle, with origin and development having the same size?).

I propose that we accept this PR first, we need to write new tests and maybe start a discussion on how to handle all the conflict collision, especially when dealing with very special cases.

[henrydingliu]

i don't think secondary_rank is causing the problem. it's optional. you can simply delete obj.iloc[...,:-1,:-1] from the failing line.

[henrydingliu]

@jbogaardt do you recall what obj.iloc[...,:-1,:-1] was changed to elsewhere in the package to deal with scalene triangles?

[jbogaardt]

@jbogaardt do you recall what obj.iloc[...,:-1,:-1] was changed to elsewhere in the package to deal with scalene triangles?

I'm not sure, but my instinct is to rely on the logic that produces the valid entries for a scalene triangle's link_ratios:

cl.load_sample('quarterly').link_ratio.nan_triangle

[henrydingliu]

Thanks @jbogaardt. Found it in the link_ratio code.

@kennethshsu can you simply delete obj.iloc... from here? That should fix the issue, as secondary_rank is optional. I'll find time this week to add back the right secondary rank.

[kennethshsu]

Yap sounds good, thanks Henry, I think that's a good temporary solution for now.

I added a couple more tests to hopefully catch this. Once secondary_rank properly ranks using the underlying triangle, we should be able to pass test_new_drop_10.

Do we need another round of code review? I know this request is technically already approved. But @jbogaardt, if it looks good feel free to merge!

[kennethshsu]

By the way @henrydingliu, if you need a triangle to play with as described in your scenario:

	12	24	36	48	60
1981	100.0	200.0
1982	200.0	200.0
1983	300.0	300.0
1984	400.0	800.0
1985	500.0

data = {
    "valuation": [
        1981,
        1982,
        1983,
        1984,
        1985,
        1982,
        1983,
        1984,
        1985,
    ],
    "origin": [
        1981,
        1982,
        1983,
        1984,
        1985,
        1981,
        1982,
        1983,
        1984,
    ],
    "values": [
        100,
        200,
        300,
        400,
        500,
        200,
        200,
        300,
        800,
    ],
}

tri = cl.Triangle(
    pd.DataFrame(data),
    origin="origin",
    development="valuation",
    columns=["values"],
    cumulative=True,
)
tri