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

Notebooks update #1634

Merged
merged 8 commits into from
Nov 14, 2023
Merged

Notebooks update #1634

merged 8 commits into from
Nov 14, 2023

Conversation

shaydeci
Copy link
Contributor

Segmentation: transfer learning, quickstart and custom dataset connection.
Updated readme links + makefile

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.

I didnt manage to comment directly on the notebooks, so here are a few notes

  1. This cell doesnt print anything. Is it on purpose (cleared) or unintended ?
from prettyformatter import pprint

print('Dataloader parameters:')
pprint(train_loader.dataloader_params)
print('Dataset parameters')
pprint(train_loader.dataset.dataset_params)

*Note: I just saw that in transfer_learning_semantic_segmentation.ipynb is it shown but in the next cell
image

  1. We are not using infery, so lett's remove it
! pip install -qq infery

README.md Outdated Show resolved Hide resolved
@shaydeci
Copy link
Contributor Author

I didnt manage to comment directly on the notebooks, so here are a few notes

  1. This cell doesnt print anything. Is it on purpose (cleared) or unintended ?
from prettyformatter import pprint

print('Dataloader parameters:')
pprint(train_loader.dataloader_params)
print('Dataset parameters')
pprint(train_loader.dataset.dataset_params)

*Note: I just saw that in transfer_learning_semantic_segmentation.ipynb is it shown but in the next cell image

  1. We are not using infery, so lett's remove it
! pip install -qq infery

Removed infery refs in 5d2c93f

@BloodAxe
Copy link
Contributor

Few notes on my end:

  1. prettyformatter is not needed and we can remove it, since there is pprint package with is supported in all python 3.x versions and does the same stuff as prettyformatter

  2. Printing to stdout is broken in our notebooks, when using DataLoaders, I have a fix for that which I can do a

  3. Quickstart segmentation notebook uses ema setting without explicitly specifying decay_type, which I think we should change to silence the warning

  4. Quickstart segmentation notebook installs onnx-simplifier without any version specifier, which is a) bad practice b) unnecessary since we have onnx-simplifier already part of SG dependencies. (See section 7) I suggest removing this install.

  5. Quickstart segmentation notebook installs torch 1.12 (See section 7) without saying a word why we ask for this specific version. I suggest removing this install as well since it's probably comes from very very old times.

@BloodAxe
Copy link
Contributor

  1. Export call uses opset_version=11 which is pretty old, suggest to remove this explicit opset argument at all.

@BloodAxe
Copy link
Contributor

segmentation_connect_custom_dataset.ipynb

  1. Same with prettyformatter
  2. Strange naming of sections 2.B Create Torch Dataset & 2.C Create Torch Dataloader. Why not 2.1 / 2.2? My first take was this some kind of typo
  3. Notebook emits warning DeprecationWarning: Object name cross_entropyis now deprecated. Please replace it withCrossEntropyLoss.

@BloodAxe
Copy link
Contributor

notebooks/transfer_learning_semantic_segmentation.ipynb

  1. prettyformatter

  2. SUPERVISELY_DATASET_DOWNLOAD_PATH="/home/data" has two issues. First - is requires Linux/Mac OS, second - it assumes that /home is writable which may not be the case (Since it's home folder for all users and not the current one). Let's avoid using absolute directories and use os.cwd() or simply ./ and build our path from it instead. We want to follow the rule that notebook should be able to be executed "as is" without any changes in cells.

  3. supervisely_dataset_dir_path should be constructed using os.path.join or pathlib.Path and not via string concatenation. It's just a bad practice.

  4. Notebook emits DeprecationWarning: Object name bce_dice_lossis now deprecated. Please replace it withBCEDiceLoss.

  5. Notebook emits WARNING - ema.py - Parameter decayis not specified for EMA params. Please specifydecay parameter explicitly in your config

@BloodAxe BloodAxe merged commit 58ec6ae into master Nov 14, 2023
8 checks passed
@BloodAxe BloodAxe deleted the feature/SG-1191_quickstart_segmentation branch November 14, 2023 07:59
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.

3 participants