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

Renaming and saving run results #2696

Merged
merged 24 commits into from
Mar 10, 2023
Merged

Renaming and saving run results #2696

merged 24 commits into from
Mar 10, 2023

Conversation

brimoor
Copy link
Contributor

@brimoor brimoor commented Feb 28, 2023

Change log

  • Adds support for renaming runs via rename_annotation_run(), rename_evaluation(), and rename_brain_run()
  • Adds an optional type argument to list_annotation_runs(), list_evaluations() and list_brain_runs() to retrieve runs of specific type (eg similarity indexes or visualizations within the full set of brain keys)
  • Adds save() method to all RunResults objects that allows for saving updates to results to the database

@brimoor brimoor requested a review from a team February 28, 2023 05:32
@brimoor brimoor self-assigned this Feb 28, 2023
@brimoor brimoor added the enhancement Code enhancement label Feb 28, 2023
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Patch coverage: 74.28% and project coverage change: -12.82 ⚠️

Comparison is base (02bb409) 61.40% compared to head (4e907a1) 48.58%.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #2696       +/-   ##
============================================
- Coverage    61.40%   48.58%   -12.82%     
============================================
  Files          256      236       -20     
  Lines        42954    32146    -10808     
  Branches       349      349               
============================================
- Hits         26374    15619    -10755     
+ Misses       16580    16527       -53     
Flag Coverage Δ
app 48.58% <74.28%> (+0.16%) ⬆️
python ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/packages/state/src/recoil/aggregations.ts 34.17% <0.00%> (ø)
app/packages/state/src/recoil/selectors.ts 45.92% <48.48%> (-3.49%) ⬇️
...es/components/src/components/ExternalLink/index.ts 100.00% <100.00%> (ø)
...ges/components/src/components/IconButton/index.tsx 100.00% <100.00%> (ø)
...ages/components/src/components/Tooltip/Tooltip.tsx 23.94% <100.00%> (+6.61%) ⬆️
app/packages/components/src/components/index.ts 100.00% <100.00%> (ø)
...mutations/__generated__/setViewMutation.graphql.ts 100.00% <100.00%> (ø)
app/packages/relay/src/mutations/setView.ts 100.00% <100.00%> (ø)
app/packages/state/src/recoil/atoms.ts 89.42% <100.00%> (-0.07%) ⬇️
app/packages/state/src/recoil/types.ts 100.00% <100.00%> (ø)
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@brimoor brimoor changed the title [WIP] adding save() method to run results Adding save() method to run results Mar 2, 2023
@brimoor brimoor marked this pull request as ready for review March 2, 2023 16:55
@brimoor brimoor changed the base branch from develop to feature/similarity March 2, 2023 20:14
Base automatically changed from feature/similarity to develop March 2, 2023 20:36
@brimoor brimoor changed the title Adding save() method to run results Renaming and saving run results Mar 3, 2023
Copy link
Contributor

@swheaton swheaton left a comment

Choose a reason for hiding this comment

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

just one comment about repeated code.
lgtm other than that

@@ -163,12 +164,33 @@ def get_fields(self, samples, eval_key):

return fields

def rename(self, samples, eval_key, new_eval_key):
Copy link
Contributor

Choose a reason for hiding this comment

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

Saw this same method above in multiple classes, can we extract it out somewhere to remain DRY ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following along with how the cleanup() and get_fields() methods are implemented explicitly on each evaluation protocol.

The implementations for each protocol are currently similar, but there's not an inherent reason why they would always be the same, so I think some repeated code is okay in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

See what you're saying but can't say I agree with the logic. I feel like this is why when we add something to OSS backend we will have multiple places we have to add it to. Which is probably fine if you're making the edit but anyone else might easily miss a spot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it currently is, there are XXXEvaluation base classes for each type of evaluation protocol: classification, detection, regression, segmentation.

The abstraction to remove duplicate code would be "this class of evaluation protocol supports either sample-level or frame-level fields. When processing sample-level fields, it populates a single field whose name matches the eval_key. When processing frame-level fields, it populates a frame-level and sample-level fields whose name matches eval_key." For that class of methods, we could abstract get_fields(), rename(), and cleanup() into a mixin and share the code.

It doesn't seem that useful to me though. Sometimes when there's a bunch of fancy subclass implementations it becomes less clear how to implement a new instance of the interface (eg a new label type to evaluate) because it seems like you have to follow how the base class does some things rather than just doing whatever you need for your case.

docs/source/user_guide/brain.rst Show resolved Hide resolved
@brimoor brimoor requested a review from swheaton March 8, 2023 03:15
brimoor and others added 4 commits March 7, 2023 22:29
* adding factory methods for cuboids and rotated boxes

* documenting

* linting

* adding new label types

* adding support for overriding the shape's filled status

* always treat polygons as filled

* removing duplicate 3D label descriptions

* cleanup

* documenting point cloud datasets

* initial text similarity in app

* 782 - add see more to similarity menu, seperate by text/image

* save sortByImage and sortByText differently in atom

* update graphQL schema to get supportsPrompts of brain methods

* add warning when brainkey exists but not support text prompt

* cleanup

* fix graphql schema bug

* adding support for querying by vectors

* adds support for text prompts

* updating docstrings

* show indication when there is no brainkey

* adds support for text prompts

* update based on PR review

* copy tweaks

* only show brain keys that supportsPrompt in text mode

* refactor components and clean up

* cleanup

* break up the main component

* refactor utils and useEffect

* code pass

* fix info link

* update cls for similairty brain run condition

* fix viewbar parsing validation error

* toggle icons

* fix k input setting issue and tune icons to grey color

* rm inapplicable sort

* fix extend stage bug and viewbar casting issue

* fix export name inconsistency

* add brain run method type into graphQL and replace the cls hack for getting similarity brain methods

* convert brainMethods.config.type from list to string

* add hack back as a fallback for the sort/ route dataset.brainMethods.config.type bug

* sentence case

* linting

* apply extended stages first

* apply extended stages before filters

* add loading state after similarity run submission + correct the tooltip

* loading icon style

* path folder name de-capitalizaed to fix build issue

* path issue with build

* removing duplicate docs

* add loading icon for image search + trying to add brainmethod.config.type to data in Sort.py

* fix duplicate progress icons

* use setattr to overwrite bound

* linting

* add progress icon for imagesearch

---------

Co-authored-by: brimoor <brimoor@umich.edu>
Co-authored-by: Ritchie Martori <ritchie@voxel51.com>
Co-authored-by: Benjamin Kane <ben@tapes.co>
@brimoor brimoor merged commit 08fc683 into develop Mar 10, 2023
@brimoor brimoor deleted the feature/save-run-results branch March 10, 2023 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Code enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants