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

Feature/dg 979 support classification #149

Merged
merged 30 commits into from
Jul 19, 2023

Conversation

BloodAxe
Copy link
Contributor

@BloodAxe BloodAxe commented Jul 10, 2023

  • Adds support for classification task on public API level
  • Adds simple features (class distribution)
  • Improve ask_user method to use ipython widgets instead of stdin when launched from notebook to allow users to provide input

@shaydeci
Copy link
Contributor

shaydeci commented Jul 10, 2023

  • Adds support for classification task on public API level
  • Adds simple features (class distribution)
  • Improve ask_user method to use ipython widgets instead of stdin when launched from notebook to allow users to provide input

In SG we kept colab links rather then having the ipynb files inside the repo, I think its better that way (in case changes are introduced we will always need to update in 2 places...)

@BloodAxe
Copy link
Contributor Author

  • Adds support for classification task on public API level
  • Adds simple features (class distribution)
  • Improve ask_user method to use ipython widgets instead of stdin when launched from notebook to allow users to provide input

In SG we kept colab links rather then having the ipynb files inside the repo, I think its better that way (in case changes are introduced we will always need to update in 2 places...)

Well I can argue that this break "atomic change" concept, that you have to make a DF release first, then go over all colab files and update them by hand. From my perspective having them under git is easier to maintain. These are meant to be "quick usage examples" that people can quickly refer to.
Maybe for deep dive colabs as we did for YOLO-NAS it make sense to store them elsewhere...
But honestly - location of notebook files is the smallest question I have about this PR ):

Copy link
Contributor

@Louis-Dupont Louis-Dupont 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, I left some comments inline but nothing too important.

As a bonus, if you can add a minimalistic end-to-end test with a dummy dataset (which can be built on the fly with random images and only a couple of random classes.
We don't have such tests for segmentation and detection but I think we should asap.

@BloodAxe BloodAxe marked this pull request as ready for review July 11, 2023 12:52
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

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

a few minor comments

requirements.txt Outdated Show resolved Hide resolved
@Louis-Dupont
Copy link
Contributor

Looks like the notebook still includes this plot
image

Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

LGTM

@ofrimasad ofrimasad requested a review from shaydeci July 19, 2023 07:27
@Louis-Dupont
Copy link
Contributor

Note: We agreed on adressing the changes concerning the denormalization in a following RP

@Louis-Dupont Louis-Dupont merged commit 08f406c into master Jul 19, 2023
@Louis-Dupont Louis-Dupont deleted the feature/DG-979-support-classification branch July 19, 2023 08:51
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