Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exacteness tests and CI improvements (#73 and #74) #78

Merged
merged 58 commits into from
Apr 23, 2024
Merged

Exacteness tests and CI improvements (#73 and #74) #78

merged 58 commits into from
Apr 23, 2024

Conversation

brash6
Copy link
Collaborator

@brash6 brash6 commented Mar 20, 2024

This PR is the combinaison of the following PRs :

This is at the same time a great test of the new CI pipeline as it is supposed to be launched every time a new PR is created on the main branch.

If any change has to be done, I suggest to :

  • Create a new branch
  • Open a new PR to be merged in develop
  • Let this PR open until everything works as expected

@bthirion
Copy link
Collaborator

What is the reason for the CI failure ?

Copy link
Collaborator

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM besides CI current state.

setup.py Outdated
@@ -17,13 +17,14 @@
),
package_dir={"": "src"},
install_requires=[
'pandas>=1.2.1',
'pandas==1.2.1',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need that ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid the DataFrame has no attribute 'iteritems' error from simulation_based and mediation_g_estimator

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, well, but we cannot be stuck with a Pandas version that seem a bit old. Let's take more recent versions, but without imposing one.

@brash6
Copy link
Collaborator Author

brash6 commented Mar 21, 2024

What is the reason for the CI failure ?

One of the reason is I think because of the fact that there's an infinitesimal difference between the obtained results and the expected results so I have to increase the tolerance threshold of the pytest.approx function.

I am going to open a new PR with CI active for develop branch so that we can test everything in this new PR before reviewing that everything is working for the main branch in this actual PR

Copy link
Collaborator

@houssamzenati houssamzenati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen and read the new functions, refactoring/relocation of lines in different files, simplification of packages import, corrections.

The PR looks good to me. Thank you for this work.

@codecov-commenter
Copy link

Codecov Report

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

❗ No coverage uploaded for pull request base (main@4ebee9c). Click here to learn what that means.

Files Patch % Lines
src/med_bench/get_estimation.py 58.33% 10 Missing ⚠️
src/med_bench/utils/utils.py 77.77% 10 Missing ⚠️
src/med_bench/mediation.py 97.29% 1 Missing ⚠️

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #78   +/-   ##
=======================================
  Coverage        ?   86.53%           
=======================================
  Files           ?        6           
  Lines           ?      750           
  Branches        ?        0           
=======================================
  Hits            ?      649           
  Misses          ?      101           
  Partials        ?        0           

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

@brash6
Copy link
Collaborator Author

brash6 commented Apr 23, 2024

I think we can merge this PR
@bthirion @judithabk6 @houssamzenati @zbakhm

Copy link
Collaborator

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@brash6 brash6 merged commit 9cc2394 into main Apr 23, 2024
4 checks passed
@brash6 brash6 deleted the develop branch April 23, 2024 16:10
@brash6 brash6 restored the develop branch April 30, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants