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.

Sign up for GitHub

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

Jump to bottom

Create single table Boundaries property #400

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Create single table Boundaries property #400

merged 5 commits into from
Jul 27, 2023

Conversation

R-Palazzo
Copy link
Contributor

Resolve #391

@R-Palazzo R-Palazzo requested a review from a team as a code owner July 24, 2023 12:32
@R-Palazzo R-Palazzo removed the request for review from a team July 24, 2023 12:32
amontanez24
amontanez24 reviewed Jul 24, 2023
View reviewed changes
tests/unit/reports/single_table/_properties/test_boundary.py
class TestBoundary:

@patch('sdmetrics.reports.single_table._properties.boundary.BoundaryAdherence.compute')
def test__generate_details(self, boundary_adherence_mock):
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add one unit test where the metric is mocked to throw an error so we can test the exception handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done in e260202

amontanez24
amontanez24 approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

One comment but otherwise looks good! Can you make a branch for DiagnosticReport issues and start merging these there?

tests/unit/reports/single_table/_properties/test_boundary.py Outdated Show resolved Hide resolved
@R-Palazzo R-Palazzo changed the base branch from master to diagnostic-report-properties July 25, 2023 09:10
@R-Palazzo R-Palazzo force-pushed the issue-391-boundhary-property branch from b4f8c36 to 8ebc135 Compare July 26, 2023 09:11
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (722bcd1) 77.36% compared to head (722bcd1) 77.36%.

❗ Current head 722bcd1 differs from pull request most recent head 8ebc135. Consider uploading reports for the commit 8ebc135 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@                      Coverage Diff                      @@
##           diagnostic-report-properties     #400   +/-   ##
=============================================================
  Coverage                         77.36%   77.36%           
=============================================================
  Files                                84       84           
  Lines                              3411     3411           
=============================================================
  Hits                               2639     2639           
  Misses                              772      772

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

pvk-developer
pvk-developer approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

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

LGTM!

@R-Palazzo R-Palazzo merged commit 526c57d into diagnostic-report-properties Jul 27, 2023
45 checks passed
@R-Palazzo R-Palazzo deleted the issue-391-boundhary-property branch July 27, 2023 08:30
R-Palazzo added a commit that referenced this pull request Jul 28, 2023
@R-Palazzo
Create single table Boundaries property (#400)
5fe5da0 
* def 1

* def 2 + test

* docstring

* add test error

* docstring
R-Palazzo added a commit that referenced this pull request Aug 23, 2023
@R-Palazzo
Create single table Boundaries property (#400)
e6450eb 
* def 1

* def 2 + test

* docstring

* add test error

* docstring
@R-Palazzo R-Palazzo mentioned this pull request Aug 23, 2023
Merged
amontanez24 pushed a commit that referenced this pull request Sep 13, 2023
@R-Palazzo @amontanez24
Create single table Boundaries property (#400)
e30c467 
* def 1

* def 2 + test

* docstring

* add test error

* docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amontanez24 amontanez24 amontanez24 approved these changes

@pvk-developer pvk-developer pvk-developer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create single table Boundaries property
4 participants
@R-Palazzo @codecov-commenter @amontanez24 @pvk-developer