search

judithabk6 / med_bench

BSD 3-Clause "New" or "Revised" License
8 stars 3 forks source link

Input check #81

Open judithabk6 opened 3 months ago

judithabk6 commented 3 months ago

still a draft (I know there are commented lines to remove or this sort of things), I have issues with

I open the PR to discuss if the main direction is ok for everyone (sorry, in develop, not main)

judithabk6 commented 1 month ago

So tests pass, however I think they should not. I missed this modification of tests, https://github.com/judithabk6/med_bench/commit/4f63b8059b8baf6fa79303b02b2e188692e6a403 but it results on the exactness tests being no super useful as they do not catch modifications of the behavior of the estimators. And the threshold of 0.01 is larger than the indirect effect so it's really permissive

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.08%. Comparing base (43c118b) to head (52fe224).

Files Patch % Lines
src/med_bench/mediation.py 90.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #81 +/- ## =========================================== + Coverage 86.53% 89.08% +2.55% =========================================== Files 6 6 Lines 750 733 -17 =========================================== + Hits 649 653 +4 + Misses 101 80 -21 ```

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

judithabk6 commented 3 weeks ago

The PR looks good, but probably needs more tests.

The PR looks good, but probably needs more tests.

indeed. I think it would be nice to test each case where the check_input function raises + the output format. I don't know if you have other tests in mind @bthirion ?

As discussed @brash6 we can work on it together depending on who has time first

bthirion commented 3 weeks ago

Sorry, I would inspect uncovered lines of code.

bthirion commented 2 weeks ago

The changes LGTM. But we need a green CI...

judithabk6 commented 1 week ago

I have modified the tests to make them simpler and to split them (instead of testing several things in the same test). Not sure which one is best. Let me know, but otherwise, I think it's ok, and it will allow to merge the fixes of the CI.