Skip to content

Conversation

@scymtym
Copy link
Contributor

@scymtym scymtym commented Dec 4, 2024

Before this change, the _exclude attribute in criteria classes led to inconsistent mapping between criterion objects and logic expression objects as well a few complications which arose from handling this mismatch.

This major change in pull request is separated into two commits

  1. Introduce the logical criterion combinations with operator not and use it where appropriate.
    As a result of this change, the _exclude attribute is always False.

  2. Remove the _exclude and logic for validating and inverting it.

Both commits represent consistent states in which all tests should pass.

@codecov
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.61%. Comparing base (4c1ce41) to head (78a0237).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...n_engine/omop/criterion/combination/combination.py 44.44% 5 Missing ⚠️
execution_engine/omop/criterion/abstract.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #242      +/-   ##
==========================================
- Coverage   87.65%   87.61%   -0.04%     
==========================================
  Files          92       92              
  Lines        5897     5887      -10     
==========================================
- Hits         5169     5158      -11     
- Misses        728      729       +1     

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

Copy link
Member

@glichtner glichtner left a comment

Choose a reason for hiding this comment

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

super!

@glichtner glichtner force-pushed the remove-exclude branch 2 times, most recently from 35011dc to 7915cbb Compare December 10, 2024 11:30
these tests (in tidal volume) were not functional anyway
@glichtner glichtner merged commit 19f449d into main Dec 10, 2024
3 checks passed
@glichtner glichtner deleted the remove-exclude branch December 10, 2024 11:56
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.

3 participants