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

Add Structure property #483

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

R-Palazzo
Copy link
Contributor

CU-86ayg9qwy
Resolve #468

Let me know if this visualization works @frances-h, @npatki
Screenshot 2023-10-26 at 08 23 20

@R-Palazzo R-Palazzo requested review from frances-h and fealho October 26, 2023 14:25
@R-Palazzo R-Palazzo requested a review from a team as a code owner October 26, 2023 14:25
@R-Palazzo R-Palazzo requested review from npatki and removed request for a team October 26, 2023 14:25
@npatki
Copy link
Contributor

npatki commented Oct 26, 2023

@R-Palazzo seems OK for an MVP.

Copy link
Contributor

@npatki npatki left a comment

Choose a reason for hiding this comment

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

@R-Palazzo one small title change. I also wonder if you can share a screenshot of the tooltip (on-hover) to verify that it makes sense for the visualization.

fig = px.bar(
data_frame=self.details.dropna(subset=['Score']),
y='Score',
title=f'Data Diagnostics: Structure (Average Score={round(average_score, 2)})',
Copy link
Contributor

Choose a reason for hiding this comment

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

@R-Palazzo Title should be Data Diagnostic (singular, not plural)

We should make sure the title of these visualizations are consistent everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed in 620285f

Base automatically changed from issue-463-tableformat-metric to diagnostic_report_updates October 26, 2023 19:37
@R-Palazzo R-Palazzo force-pushed the issue-468-add-structure-property branch from b793e9f to 620285f Compare October 26, 2023 21:11
@codecov-commenter
Copy link

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (6c45928) 77.67% compared to head (620285f) 77.67%.

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

Additional details and impacted files
@@                      Coverage Diff                      @@
##           diagnostic_report_updates     #483      +/-   ##
=============================================================
- Coverage                      77.67%   77.67%   -0.01%     
=============================================================
  Files                             97       99       +2     
  Lines                           3508     3548      +40     
=============================================================
+ Hits                            2725     2756      +31     
- Misses                           783      792       +9     
Files Coverage Δ
...etrics/reports/multi_table/_properties/__init__.py 100.00% <100.00%> (ø)
...trics/reports/multi_table/_properties/structure.py 100.00% <100.00%> (ø)
...trics/reports/single_table/_properties/__init__.py 100.00% <100.00%> (ø)
...rics/reports/single_table/_properties/structure.py 72.72% <72.72%> (ø)

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

@R-Palazzo R-Palazzo requested a review from npatki October 27, 2023 12:13
Copy link
Member

@fealho fealho left a comment

Choose a reason for hiding this comment

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

Looks pretty good 👍

@R-Palazzo R-Palazzo merged commit 960e4a8 into diagnostic_report_updates Oct 31, 2023
45 checks passed
@R-Palazzo R-Palazzo deleted the issue-468-add-structure-property branch October 31, 2023 16:22
R-Palazzo added a commit that referenced this pull request Nov 2, 2023
R-Palazzo added a commit that referenced this pull request Nov 6, 2023
R-Palazzo added a commit that referenced this pull request Nov 14, 2023
R-Palazzo added a commit that referenced this pull request Nov 27, 2023
R-Palazzo added a commit that referenced this pull request Nov 27, 2023
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.

5 participants