Refac(MSplit): LVAE module - #1033
Conversation
|
I'm very open to suggestion, maybe missed something |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/microsplit_api #1033 +/- ##
======================================================
+ Coverage 83.49% 83.64% +0.14%
======================================================
Files 252 252
Lines 9254 9160 -94
======================================================
- Hits 7727 7662 -65
+ Misses 1527 1498 -29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jdeschamps
left a comment
There was a problem hiding this comment.
I did not review the PR, just had a quick look.
Could you comment on how you tested correctness? It is part of the PR template and it would be useful to know whether the existing model weights can still be loaded, and/or whether you ran some training.
|
|
||
| return z_dims | ||
|
|
||
| def set_3D(self, is_3D: bool) -> None: |
There was a problem hiding this comment.
We should also have another look at the UNet and whether this set 3D is ever used or necessary. Maybe a small clean up issue.
There was a problem hiding this comment.
Following #1038, this should not be removed and we should see what else needs to be updated for the LVAE architecture to be set to 3D.
Indeed, we need to have an HDN napari plugin for a grant.
Co-authored-by: Joran Deschamps <6367888+jdeschamps@users.noreply.github.com>
Old checkpoints trained by Ashesh won't load in any version of careamics without some manipulation with weights. We have that in reproducibility. |
|
Can you report on training performances? |
~ 31.1 / 32.2, given the full(400ep) original run produced 32/32.9 |
I think it's a substantial difference. If the performance was checked with a script similar to one in #1052, there is no seed set anywhere during training (please correct me if I'm wrong!). Can you re-run two comparison runs with the same seed, and maybe cuda deterministic and benchmark for a good measure? |
seeds are very good point, will add. As for the numbers, also forgot that those were with mmse of 1, with 50 it jumps to 31.6/32.8. Running full 400ep also |
|
As discussed, before merging we are waiting for full report of the experiments (original PSNR, this branch with seeds and mmse) and for @veegalinova tests on HDN. |
Disclaimer
Description
Note
tldr: Behavior-preserving cleanup of the LVAE model — dead code removal, layer blocks cleanup, and
models/lvae/back to pre-commits.Background - why do we need this PR?
Lot's of old commented-out code, unnecessary inheritance, dead parameters, lot's of pre-commit errors
Overview - what changed?
lvae.py,layers.py,stochastic.py,utils.py.block_type,batchnorm,conv2d_bias,res_block_kernel,merge_type, skip-padding) and their branches.ResBlockWithResampling,ResidualGatedBlock,SkipConnectionMerger.BottomUpDeterministicResBlock/TopDownDeterministicResBlockare now standalone and share a_make_pre_convhelper.models/lvae/pre-commit exclusions removedModified features or files
src/careamics/models/lvae/lvae.py,layers.py,stochastic.py,utils.py— cleanup + specialization + docstrings/typing.src/careamics/config/architectures/lvae_config.py— removed dead/brokenset_3D..pre-commit-config.yaml— narrowed the lvae exclusions tonoise_models.pyonly.tests/models/lvae/test_lvae_architecture.py— updated the (already-skipped)test_bottom_up_passto callbottomup_passafter the wrapper was inlined.Removed features or files
ResBlockWithResampling,ResidualGatedBlock,SkipConnectionMerger,ModelType,power_of_2,LVAEConfig.set_3D.Please ensure your PR meets the following requirements: