Merging mass-mapping to main #3
Conversation
| journal={arXiv preprint arXiv:1907.08175}, | ||
| year={2019} | ||
| } | ||
| This repository was forked from rcGAN by Bendel et al., with significant changes and modification made by Whitney et al. |
There was a problem hiding this comment.
Add link to original Bendel paper as well as the original rcGAN repo
| @@ -1,72 +1,25 @@ | |||
| # A Regularized Conditional GAN for Posterior Sampling in Inverse Problems [[arXiv]](https://arxiv.org/abs/2210.13389) | |||
| # Generative modelling for mass-mapping with fast uncertainty quantification [[arXiv]](https://arxiv.org/abs/2410.24197) | |||
| ## Setup | |||
There was a problem hiding this comment.
It'd be good to add a small description of the method (something like the abstract used for the paper but more synthetic). Also select one figure from the paper to illustrate the method (I'd suggest the one from the COSMOS field showing the reconstruction and std dev).
|
|
||
| # Installation | ||
|
|
||
| If in the Hypatia cluster, first run: |
There was a problem hiding this comment.
We should not mention Hypatia, but be general for a computer cluster.
| @@ -1,72 +1,25 @@ | |||
| # A Regularized Conditional GAN for Posterior Sampling in Inverse Problems [[arXiv]](https://arxiv.org/abs/2210.13389) | |||
| # Generative modelling for mass-mapping with fast uncertainty quantification [[arXiv]](https://arxiv.org/abs/2410.24197) | |||
| ## Setup | |||
| See ```docs/setup.md``` for basic environment setup instructions. | |||
There was a problem hiding this comment.
This file does not exist
| @@ -1,72 +1,25 @@ | |||
| # A Regularized Conditional GAN for Posterior Sampling in Inverse Problems [[arXiv]](https://arxiv.org/abs/2210.13389) | |||
| # Generative modelling for mass-mapping with fast uncertainty quantification [[arXiv]](https://arxiv.org/abs/2410.24197) | |||
| ## Setup | |||
| See ```docs/setup.md``` for basic environment setup instructions. | |||
|
|
|||
There was a problem hiding this comment.
You need to add a reference to the comments.md file where we explain how to install the model and some other useful stuff. There are two option, mention what you can find on that file and provide a link or merge the info of that file in the readme
| @@ -0,0 +1,48 @@ | |||
| # Reproducing our MRI results | |||
There was a problem hiding this comment.
Update MRI to mass mapping
|
|
||
| ## Extending the Code | ||
| See ```docs/new_applications.md``` for basic instructions on how to extend the code to your application. | ||
| ## Reproducing the our Results |
| title = {co-mod-gan-pytorch}, | ||
| year = 2022 | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
It'd be good to add a reproducibility section and links to Zenodo. It'd be good to find there the trained GAN used to generate the results. Also, some samples and reconstructions of the GAN.
Should we include the simulations in a zenodo? It depends on the size of the sims. If they are too big, you could just upload some of them to have a representative set.
| See ```docs/new_applications.md``` for basic instructions on how to extend the code to your application. | ||
| ## Reproducing the our Results | ||
| ### | ||
| See ```docs/mass_mapping.md``` for instructions on how to setup and reproduce our COSMOS results. | ||
|
|
There was a problem hiding this comment.
Could you coordinate with Matthijs so that the radio application of the GAN is also updated on this main branch. It'd be good to have everything to reproduce the results of both papers in a single repo.
There was a problem hiding this comment.
Shouldn't the MRI job be on the mri_utils directory
There was a problem hiding this comment.
Shouldn't the MRI job be on the mri_utils directory?
There was a problem hiding this comment.
Why it finishes by copy.sh?
Shouldn't the MRI job be on the mri_utils directory?
There was a problem hiding this comment.
Shouldn't the MRI job be on the mri_utils directory?
There was a problem hiding this comment.
It should be good to have a small readme file in the mass_map_utils/notebooks/ directory such that there's a phrase describing each notebook in the directory.
| mean = data.mean() | ||
| std = data.std() | ||
|
|
||
| return normalize(data, mean, std, eps), mean, std | ||
|
|
||
| def normalise_complex( |
There was a problem hiding this comment.
You should add a docstring on this function and the one below to specify what those two default values represent. It would also be good to add a phrase describing what exactly each of the functions do.
| # Load the correct model | ||
| if cfg.experience == 'mri': | ||
| dm = MRIDataModule(cfg) | ||
| model = rcGAN(cfg, args.exp_name, args.num_gpus) |
There was a problem hiding this comment.
It seems that the rcGAN disappeared from the from models.lightning.rcGAN import rcGAN
We should be consistent. Or we remove this from here or add the rcGAN module again so that it can be used as suggested by this line and the config files.
| @@ -152,7 +152,7 @@ def _get_embed_im(self, multi_coil_inp, mean, std, maps): | |||
|
|
|||
| S = sp.linop.Multiply((self.args.im_size, self.args.im_size), maps[i]) | |||
|
|
|||
| im = torch.tensor(S.H * tensor_to_complex_np(unnormal_im.cpu())).abs().cuda() | |||
| im = torch.real(torch.tensor(S.H * tensor_to_complex_np(unnormal_im.cpu()))).cuda() | |||
There was a problem hiding this comment.
It'd be good to add a comment to the code to explain this modification. Eventually, leave the previous line commented to show the previous implementation.
Also, this function will probably be used in the radio problem, so it'd be good to check with Matthijs what makes more sense to use in his problem. This change could be a potential bug for his application if it goes unnoticed.
The full mass-mapping pipeline used for the paper, with updated notebooks and configs. No bugs as this worked from training all the way through to plotting/validation.