search

casact / chainladder-python

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

make _set_weight_func return a triangle #389

Closed henrydingliu closed 1 year ago

henrydingliu commented 2 years ago

a triangle looks much better. easier to do this during fitting, since the fitted development estimator doesn't keep odims.

jbogaardt commented 2 years ago

Hi @henrydingliu, this PR makes some of your new unit tests fail. These can be seen from clicking any of the Details links above. Can you address this before accepting the merge into main branch?

=========================== short test summary info ============================
FAILED chainladder/development/tests/test_development.py::test_new_drop_1[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_1[sparse_only_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_2[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_2[sparse_only_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_3[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_3[sparse_only_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_4[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_4[sparse_only_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_5[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_5[sparse_only_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_6[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_6[sparse_only_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_7[normal_run]
FAILED chainladder/development/tests/test_development.py::test_new_drop_7[sparse_only_run]
==== 14 failed, 574 passed, 11 xfailed, 2387 warnings in 300.91s (0:05:00) =====
henrydingliu commented 2 years ago

@jbogaardt I added #390 after seeing these fails.

E TypeError: no implementation found for 'numpy.array_equal' on types that implement __array_function__: [<class 'chainladder.core.triangle.Triangle'>, <class 'numpy.ndarray'>]

The issue is that the test functions for _set_weight_func expects an ndarray output. This commit changed it to a Triangle output. What's the proper steps for checking in code that messes with tests? delete the relevant tests first? Close both then re-submit as a single commit?

jbogaardt commented 1 year ago

What's the proper steps for checking in code that messes with tests? delete the relevant tests first? Close both then re-submit as a single commit?

Great question! I've worked under the notion that code changes and tests within a single pull request should remain in sync and not introduce test failures. This keeps master branch as clean as possible for others to branch from. This also gives latitude to you to modify or delete tests as you see fit to have a clean PR. I do watch for code coverage too which is currently at 84%. While deleting every test will make things pass, our code coverage would plummet. So, maintaining a near consistent or improved code coverage while having all tests pass is ideal for clean PR and will limit peer review time needed.