Skip to content

HDN lightning example - #40

Open
CatEek wants to merge 6 commits into
mainfrom
hdn_lightning_example
Open

HDN lightning example#40
CatEek wants to merge 6 commits into
mainfrom
hdn_lightning_example

Conversation

@CatEek

@CatEek CatEek commented Sep 16, 2025

Copy link
Copy Markdown

Adds the lightning API notebook for HDN.

Uses newly proposed configuration pipeline see careamics #511.

Performance haven't been tested.

@jdeschamps jdeschamps left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are two small changes to the text that are requested, and one big question about the N2V notebook disappearing.

I also made notes for API changes but they are not related to this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you delete the Noise2Void example? Beyond being unnecessary, that will also make the website crash.

"The BSD68 dataset was adapted from K. Zhang et al (TIP, 2017) and is composed of natural\n",
"images. The noise was artificially added, allowing for quantitative comparisons with the\n",
"ground truth, one of the benchmark used in many denoising publications. Here, we check \n",
"the performances of Noise2Void using the Lightning API of CAREamics.\n",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"the performances of Noise2Void using the Lightning API of CAREamics.\n",
"the performances of HDN using the Lightning API of CAREamics.\n",

" experiment_name=\"bsd68_hdn\",\n",
" data_type=\"tiff\",\n",
" axes=\"SYX\",\n",
" z_dims=[32] * 4,\n",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am making a comment here for reference, but it does not belong to this PR.

Note: It is horrible for users to have to do something like [32] * 4. What does it mean? Why 4? etc. The convenience function should make it easy. For instance with a parameter (I suppose that's what it is about) n_levels or n_depth (should be similar to what is in the config, provided it has an informative name).

" patch_size=(128, 128),\n",
" batch_size=64,\n",
" num_epochs=5,\n",
" predict_logvar=\"pixelwise\",\n",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: If I recall, this could be a bool.

Comment on lines +182 to +183
"Note that here we modify the prediction loop, but this will be changed in the near\n",
"future."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"Note that here we modify the prediction loop, but this will be changed in the near\n",
"future."

" data_type=\"tiff\",\n",
" axes=\"YX\",\n",
" batch_size=1,\n",
" tta_transforms=False, #not implemented\n",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: kick out tta parameters fully until we implement it.

@jdeschamps

Copy link
Copy Markdown
Member

Closes #38

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.

2 participants