Skip to content

Conversation

phil-scale
Copy link
Contributor

@phil-scale phil-scale commented Jan 3, 2022

Adds IoU, precision, and recall metrics to the Nucleus repository. Creates framework to support more general metrics.

Note: adding the Shapely package also adds a dependency on the geos library, which can be installed in MacOS using brew install geos.

Design doc here: https://docs.google.com/document/d/1uOjZQ9wRMdpsOaYN_w6eTqPvQu0Nwtl5wRNj0HkL0EU/edit#heading=h.raendxyxrl42
ch318548
ch303550

@phil-scale phil-scale requested review from pfmark, yixu34, sasha-scale, a team and ardila January 3, 2022 18:12
@shortcut-integration
Copy link

@phil-scale
Copy link
Contributor Author

phil-scale commented Jan 3, 2022

Note: this bumps the python requirement to >= 3.7, < 3.11 in order to support scipy - is that ok?

Edit: Scipy, numpy, and Shapely all will be installed only if the python version is >= 3.7, < 3.11. The required python version is still ^3.6

@phil-scale phil-scale requested a review from gatli January 3, 2022 19:36
Copy link
Contributor

@gatli gatli left a comment

Choose a reason for hiding this comment

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

Very clear code to read 🙂

I've requested changes since we need to be sure that Python 3.6 users see a warning message to upgrade. Until then I'm not comfortable rolling this out since I know that python3.6 is default in some widely used distros.

@phil-scale
Copy link
Contributor Author

Very clear code to read 🙂

I've requested changes since we need to be sure that Python 3.6 users see a warning message to upgrade. Until then I'm not comfortable rolling this out since I know that python3.6 is default in some widely used distros.

Moved the new dependencies to dev only, with the Python requirement applied specifically to those dependencies.

def __init__(
self,
enforce_label_match: bool = False,
iou_threshold: float = 0.0,
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to have defaults for the IOU and confidence thresholds? Those seem like things we'd want the user to specify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking is that we can expose information about what defaults are used in Model CI in two ways:

  1. Currently, we can keep these defaults in code so that users who want to see what the values are can find them directly here as defaults.
  2. Once the UI shows the source code of evaluation functions, we can get rid of the defaults here and instead show them in the source code (e.g. PolygonIOU(enforce_label_match=True, iou_threshold=0.2, confidence_threshold=0.5)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, I'll add a TODO here in polygon_metrics.py to remove the defaults once we can surface these defaults more directly to the user.

def __init__(
self,
enforce_label_match: bool = False,
iou_threshold: float = 0.5,
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Also wondering why this is 0.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that in Nucleus and the most commonly reported metrics, it's usually 0.5 by default. Also if we're exposing these evaluation functions to users, we would want users to know the default values that are actually used in Model CI.

Copy link
Contributor

@gatli gatli left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🔥

These tests are failing on master as well so they shouldn't block you. Hopefully I'll have a fix today and then you can simply rebase on top.

@gatli
Copy link
Contributor

gatli commented Jan 11, 2022

Rebased on top of master ...

@phil-scale phil-scale merged commit b329768 into master Jan 11, 2022
@phil-scale phil-scale deleted the phil/metrics branch January 11, 2022 18:01
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.

4 participants