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

2024 Review #5

Open
neptunes5thmoon opened this issue Aug 20, 2024 · 0 comments
Open

2024 Review #5

neptunes5thmoon opened this issue Aug 20, 2024 · 0 comments

Comments

@neptunes5thmoon
Copy link
Contributor

Finished reviewing (looked at ben/denoisplit as per mattermost).
01 and 04 need some work I think but for the other ones only minor comments:

Installation

  • end up in a different directory after running source setup.sh
  • recommended is putting data on our s3 bucket and download from there
  • dependencies of denoiSplit not installed

01_CARE

  • missing colored boxes for checkpoints, exercises etc. in the exercise notebook
  • still got TODOs in there (most importantly: missing scaffolding for some tasks)
  • import for augment_batch and normalize is at the very beginning and defined by you. Maybe add a comment that makes clear that it's provided by you and add a cell with ?augment_batch and ?normalize at task 1, similarly for denormalize in task 3
  • agree that calculation of means and stds should go in the function
  • they already know about data loaders in principle
  • the unet should come from here: https://github.com/dlmbl/dlmbl-unet
  • IMO importing torch.nn and torch.optim would be better than importing with as nn
  • I got NaNs in my loss when my lr was too high
  • something with the training is not working for me - validation loss is always the same

02_Noise2Void

  • config explanation: I think axes being "YX" warrants more explanation (e.g. they might get 2D but be thrown for a loop by it not being "XY" )
  • getting some warnings during training
  • Task2: tensorboard is already started
  • in export: TypeError: CAREamist.export_to_bmz() got an unexpected keyword argument 'path' (parameter is called path_to_archive)
  • Task 5: detail but data is already downloaded, so change text to "loaded in the following cell"

03_COSDD

  • put explanation for dimensions before or in the task instead of after
  • still has two TODOs (figure and pretrained model)
  • in tensorboard metrics: 20 min in text instead of 10 min
  • if you can add tensorboard logs for the pretrained models that'd be great
  • Bonus
    • import is wrong
    • also still TODO: add pretrained
    • device is not defined

04_DenoiSplit

  • sys path extension isn't right
  • missing dependency albumentations
  • I assume this isn't done? Very light on details of what's happening
  • exercise version has answers to questions

05 Noise2Noise

  • text: "regression exercises" -> restoration exercises
  • Task1: typo vizualize -> visualize
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

No branches or pull requests

1 participant