-
Notifications
You must be signed in to change notification settings - Fork 395
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
Vision dashboard backend without data characteristics tab #1668
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1668 +/- ##
==========================================
- Coverage 89.04% 88.88% -0.17%
==========================================
Files 105 105
Lines 5534 5553 +19
==========================================
+ Hits 4928 4936 +8
- Misses 606 617 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fdeba1e
to
dc3552b
Compare
2 similar comments
dc3552b
to
4452ff2
Compare
4452ff2
to
e3819e0
Compare
raiwidgets/raiwidgets/__version__.py
Outdated
@@ -3,4 +3,4 @@ | |||
|
|||
# Version will be populated during release from version.cfg file | |||
# by running `yarn auto-version` | |||
version = "0.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this version is populated by the release script, so make sure to remove this when merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like you need to re-add back the 0.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this for you and pushed to your PR
libs/interpret-vision/src/lib/VisionExplanationDashboard/Controls/Flyout.styles.ts
Outdated
Show resolved
Hide resolved
libs/interpret-vision/src/lib/VisionExplanationDashboard/Controls/ImageList.tsx
Outdated
Show resolved
Hide resolved
libs/interpret-vision/src/lib/VisionExplanationDashboard/Controls/ImageList.tsx
Show resolved
Hide resolved
return ret; | ||
if (itemIndex === 0) { | ||
this.columnCount = Math.ceil(visibleRect.width / this.props.imageDim); | ||
this.rowHeight = Math.floor(visibleRect.width / this.columnCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can visibleRect change from image to image? I'm wondering actually if these can all be variables on state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it changes, but my movtivation behind not making columnCount and rowHeight state variables was to not trigger rerenders when they're calculated. Is it preferable to only use state variables?
libs/interpret-vision/src/lib/VisionExplanationDashboard/Controls/PageSizeSelectors.tsx
Outdated
Show resolved
Hide resolved
libs/interpret-vision/src/lib/VisionExplanationDashboard/Controls/TabsView.tsx
Outdated
Show resolved
Hide resolved
{t.key === GlobalTabKeys.VisionTab && | ||
this.props.visionModelExplanationData && ( | ||
<> | ||
<div className={classNames.sectionHeader}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to use Stack instead of div, like here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just copying all of the other components which use div and h3. Should I still use Stack even if all the other components aren't, or should I update them all to be Stacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please use Stack wherever possible. The other components/examples are probably wrong if they use div. Stack is essentially div but with fluentui sprinkled in to fit our accessibility/style requirements. So please try to use Stack wherever possible instead of div.
raiwidgets/raiwidgets/__version__.py
Outdated
@@ -3,4 +3,4 @@ | |||
|
|||
# Version will be populated during release from version.cfg file | |||
# by running `yarn auto-version` | |||
version = "0.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this version is populated by the release script, so make sure to remove this when merging
@@ -3,4 +3,4 @@ | |||
|
|||
# Version will be populated during release from version.cfg file | |||
# by running `yarn auto-version` | |||
version = "0.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh it looks like this hasn't been resolved yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this for you and pushed to your PR
e3819e0
to
60024c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Will merge this now that you've fixed all comments.
60024c2
to
eae618b
Compare
The vision dashboard backend without the data characteristics tab so as to make the PR smaller.
The backend functionality and additional fixups include:
Description
Checklist