Encoder/decoder integration #667

yashpatel5400 · 2022-08-08T15:07:11Z

(Same as the previous PR, but cleaned up and now with all the tests passing)

codecov · 2022-08-08T15:18:39Z

Codecov Report

Merging #667 (231e3c4) into master (434cb60) will increase coverage by 8.10%.
The diff coverage is 91.95%.

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
+ Coverage   79.09%   87.19%   +8.10%     
==========================================
  Files          24       25       +1     
  Lines        3506     3695     +189     
==========================================
+ Hits         2773     3222     +449     
+ Misses        733      473     -260

Flag	Coverage Δ
unittests	`87.19% <91.95%> (+8.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bliss/catalog.py	`84.12% <ø> (ø)`
bliss/encoder.py	`84.54% <37.50%> (-13.16%)`	⬇️
bliss/models/galsim_decoder.py	`89.34% <94.87%> (+1.63%)`	⬆️
bliss/models/psf_decoder.py	`95.40% <95.40%> (ø)`
bliss/models/decoder.py	`97.87% <100.00%> (+0.81%)`	⬆️
bliss/models/galsim_encoder.py	`21.59% <100.00%> (+21.59%)`	⬆️
bliss/models/prior.py	`90.60% <100.00%> (+3.21%)`	⬆️
bliss/models/lensing_binary_encoder.py	`76.37% <0.00%> (+76.37%)`	⬆️
bliss/models/lens_encoder.py	`81.00% <0.00%> (+81.00%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jeff-regier · 2022-08-09T15:37:20Z

bliss/encoder.py

+ else:
+ lensed_galaxy_bools = (
+ torch.rand_like(lensed_galaxy_probs) > 0.5
+ ).float() * is_on_array.unsqueeze(-1)


Black auto formatting isn't so great for long lines. It's often clearer to create a temporary/intermediate variable to reduce the length of long lines

ismael-mendoza

Thanks @yashpatel5400 the code looks really good. My biggest suggestion is on the if statements scattered in the decoder related to the lens_params which I think will make the code harder to read/maintain in the long run. In some cases they can also prevent code optimization.

Do you think there is a way we can get rid of them (I was imagining a separate LensTileDecoder but maybe this is unfeasible)? Let's discuss over slack or in-person if you are around west hall when you have a chance.

The other comments are pretty minor and related to black formatting mostly

ismael-mendoza · 2022-08-09T17:29:43Z

bliss/encoder.py

+ if deterministic:
+ lensed_galaxy_bools = (
+ lensed_galaxy_probs > 0.5
+ ).float() * is_on_array.unsqueeze(-1)


better to split these lines into multiple statements (one per line)

ismael-mendoza · 2022-08-09T17:30:11Z

bliss/encoder.py

+ lensed_galaxy_bools = (
+ torch.rand_like(lensed_galaxy_probs) > 0.5
+ ).float() * is_on_array.unsqueeze(-1)
+


ismael-mendoza · 2022-08-09T17:30:40Z

bliss/encoder.py

+ lensed_galaxy_bools *= (
+ galaxy_bools # currently only support lensing where galaxy is present
+ )


Better to put the comment in a separate line to avoid black formatting

ismael-mendoza · 2022-08-09T17:31:34Z

bliss/encoder.py

+ tile_samples.update(
+ {
+ "lensed_galaxy_bools": lensed_galaxy_bools,
+ "lensed_galaxy_probs": lensed_galaxy_probs,
+ }
+ )


you might consider saving some lines by doing e.g. tile_samples['lensed_galaxy_bools'] = lensed_galaxy_bools instead

ismael-mendoza · 2022-08-09T17:39:46Z

bliss/models/psf_decoder.py

+ n_bands: int = 1,
+ psf_image_file: Optional[str] = None,
+ psf_params_file: Optional[str] = None,
+ psf_slen: Optional[int] = None,


is this really optional?

ismael-mendoza · 2022-08-09T17:46:06Z

bliss/models/prior.py

+ catalog_params.update(
+ {
+ "lensed_galaxy_bools": lensed_galaxy_bools,
+ "lens_params": lens_params,
+ }
+ )


save some lines by catalog_params['lens_params'] = lens_params

ismael-mendoza · 2022-08-09T17:47:15Z

bliss/models/prior.py

+ galaxy_bools, star_bools, lensed_galaxy_bools = self._sample_n_galaxies_and_stars(
+ is_on_array
+ )


I would make a new function _sample_n_lens or something like that which takes is_on_array and galaxy_bool and returns lensed_galaxy_bools

ismael-mendoza · 2022-08-09T17:47:46Z

bliss/models/galsim_encoder.py

@@ -190,7 +190,7 @@ def encode(self, image_ptiles: Tensor, tile_locs: Tensor) -> Tensor:
 ms=max_sources,
 )

- def sample(self, image_ptiles: Tensor, tile_locs: Tensor):
+ def sample(self, image_ptiles: Tensor, tile_locs: Tensor, deterministic: Optional[bool]):


why did you add this deterministic flag?

This was just to match the other GalaxyEncoder (since we support both in encoder.py)

ismael-mendoza · 2022-08-09T17:51:47Z

bliss/models/decoder.py

@@ -195,6 +195,15 @@ def _render_ptiles(self, tile_catalog: TileCatalog) -> Tensor:
 tile_catalog["star_fluxes"], "b nth ntw s band -> (b nth ntw) s band"
 )

+ if "lens_params" in tile_catalog:


we should talk about avoiding this if statements scattered throughout the code, in general tihs makes the code harder to read and maintain. I was imagining separate lens_decoder just like we have for the stars would be cleaner to maintain. Ofc it would have to be conditioned on the galaxy_bools or something. Could you tell me if this is possible ?

ismael-mendoza · 2022-08-09T17:52:16Z

bliss/models/decoder.py

@@ -580,7 +503,15 @@ def _render_single_galaxies(self, galaxy_params, galaxy_bools):

 # forward only galaxies that are on!
 # no background
- gal_on = self.galaxy_decoder(z[b == 1])
+ if lens_params is not None:


another if statement on lens_params, we should talk about whether we can remove this...

review-notebook-app · 2022-08-14T20:54:03Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

yashpatel5400 · 2022-08-14T22:31:22Z

alright, think I've addressed all the comments from above!

jeff-regier

Great!

jeff-regier · 2022-08-09T15:39:57Z

bliss/models/prior.py

+ # currently only support lensing where galaxy is present
+ lensed_galaxy_bools = uniform < self.prob_lensed_galaxy
+ lensed_galaxy_bools = (
+ lensed_galaxy_bools * galaxy_bools * is_on_array.unsqueeze(-1)


Can we use *= here so it all fits on one line?

yashpatel5400 added 3 commits August 8, 2022 11:03

Abstract PSF loading functionality

2c26870

Add lensing to core decoder

7327148

Integrate lensing into overall encoder

350d136

jeff-regier requested review from ismael-mendoza and dereklhansen August 8, 2022 15:50

yashpatel5400 force-pushed the integrate branch 5 times, most recently from 37daece to 6e1a14a Compare August 8, 2022 22:37

jeff-regier reviewed Aug 9, 2022

View reviewed changes

ismael-mendoza requested changes Aug 9, 2022

View reviewed changes

yashpatel5400 force-pushed the integrate branch from 6e1a14a to fc8cf79 Compare August 14, 2022 20:53

Add lensing tests

231e3c4

yashpatel5400 force-pushed the integrate branch 2 times, most recently from ea6ad4d to 231e3c4 Compare August 14, 2022 22:26

yashpatel5400 requested a review from ismael-mendoza August 14, 2022 22:31

jeff-regier self-requested a review August 15, 2022 01:25

jeff-regier approved these changes Aug 15, 2022

View reviewed changes

jeff-regier merged commit 832893c into prob-ml:master Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encoder/decoder integration #667

Encoder/decoder integration #667

yashpatel5400 commented Aug 8, 2022

codecov bot commented Aug 8, 2022 •

edited

Loading

jeff-regier Aug 9, 2022

ismael-mendoza left a comment

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

yashpatel5400 Aug 14, 2022

ismael-mendoza Aug 9, 2022

ismael-mendoza Aug 9, 2022

review-notebook-app bot commented Aug 14, 2022

yashpatel5400 commented Aug 14, 2022

jeff-regier left a comment

jeff-regier Aug 9, 2022

Encoder/decoder integration #667

Encoder/decoder integration #667

Conversation

yashpatel5400 commented Aug 8, 2022

codecov bot commented Aug 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ismael-mendoza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

review-notebook-app bot commented Aug 14, 2022

yashpatel5400 commented Aug 14, 2022

jeff-regier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2022 •

edited

Loading