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 frontend support for loading overlay masks from disk #2358

Merged

Conversation

sashankaryal
Copy link
Contributor

What changes are proposed in this pull request?

Frontend support for loading overlay masks from disk

How is this patch tested? If it is not, please explain why.

Tested locally.

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

Refer to #2301

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@sashankaryal sashankaryal added the feature Work on a feature request label Nov 28, 2022
@sashankaryal sashankaryal self-assigned this Nov 28, 2022
@sashankaryal sashankaryal force-pushed the feature/file-segmentation branch 2 times, most recently from c9dcf4c to a778d73 Compare November 29, 2022 01:36
Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

Looks good! Left some stylistic suggestions. Regardless, I assume this will be refactored some in the async work

const LABELS = new Set(VALID_LABEL_TYPES);
const ALL_VALID_LABELS = new Set(VALID_LABEL_TYPES);

const LABELS_THAT_CAN_HAVE_OVERLAYS_ON_DISK = new Set([
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose naming this something like ARRAY_LABELS and including in @fiftyone/utilities with the other constants for reuse.

Object.hasOwn(label, overlayField) ||
!Object.hasOwn(label, overlayPathField)
) {
// nothing to be done
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose removing

Suggested change
// nothing to be done

Comment on lines +172 to +173
// convert absolute file path to a URL that we can "fetch" from
const overlayPngImageUrl = getSampleSrc(label[overlayPathField] as string);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe alias or rename getSampleSrc so it is self-explanatory.

Suggested change
// convert absolute file path to a URL that we can "fetch" from
const overlayPngImageUrl = getSampleSrc(label[overlayPathField] as string);
const overlayPngImageUrl = expandToURL(label[overlayPathField] as string);

* Some label types (example: segmentation, heatmap) can have their overlay data stored on-disk,
* we want to impute the relevant mask property of these labels from what's stored in the disk
*/
const imputeOverlayFromPath = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename to illustrate the comment?

Suggested change
const imputeOverlayFromPath = async (
loadArrayLabels

@sashankaryal sashankaryal merged commit 892c63e into feature/on-disk-segmentations Dec 6, 2022
@sashankaryal sashankaryal deleted the feature/file-segmentation branch December 6, 2022 02:35
@sashankaryal
Copy link
Contributor Author

Thanks for the comments, @benjaminpkane, will address them in the next unit of segmentation work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Work on a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants