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

ellipticity_ryden04 has a shape mismatch bug #328

Closed
Saransh-cpp opened this issue Oct 4, 2024 · 1 comment · Fixed by #332
Closed

ellipticity_ryden04 has a shape mismatch bug #328

Saransh-cpp opened this issue Oct 4, 2024 · 1 comment · Fixed by #332
Assignees
Labels
bug Something isn't working

Comments

@Saransh-cpp
Copy link
Member

Describe the Bug

The implementation right now keeps sampling values until it gets all positive values -

glass/glass/shapes.py

Lines 142 to 151 in 2eb3e8b

if size is None:
size = np.broadcast(gamma, sigma_gamma, mu, sigma).shape
# draw gamma and epsilon from truncated normal -- eq.s (10)-(11)
# first sample unbounded normal, then rejection sample truncation
eps = rng.normal(mu, sigma, size=size)
bad = eps > 0
while np.any(bad):
eps[bad] = rng.normal(mu, sigma, size=eps[bad].shape)
bad = eps > 0

Let's say if mu = [-1.85, -2.85], gamma = 0.89, gamma = 0.222, sigma_gamma = 0.056 -

  • size = (2,)
  • eps.shape = (2,)
  • rng.normal(mu, sigma) will have the shape (2,) because numpy broadcasts the parameters internally

If one value in eps is bad (<0) -

  • bad = [True, False]
  • eps[bad].shape = (1,)

then this will not work -

eps[bad] = rng.normal(mu, sigma, size=eps[bad].shape)

as a (2,) array cannot be resized to (1,).


This bug was hidden by the float value is not indexable error when the broadcasting rule was not present.

I am not sure what should be done here. Should we consider using something like truncated normal here?

To Reproduce

from glass import ellipticity_ryden04


while True:
    ellipticity_ryden04([-1.85, -2.85], 0.89, 0.222, 0.056)  # rerun till it errors out

Expected Behaviour

No shape mismatch error.

Actual Behaviour

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
      1 while True:
----> 2     ellipticity_ryden04([-1.85, -2.85], 0.89, 0.222, 0.056)

File ~/Code/UCL/glass/glass/shapes.py:146, in ellipticity_ryden04(mu, sigma, gamma, sigma_gamma, size, rng)
    144 bad = eps > 0
    145 while np.any(bad):
--> 146     eps[bad] = rng.normal(mu, sigma, size=eps[bad].shape)
    147     bad = eps > 0
    148 gam = rng.normal(gamma, sigma_gamma, size=size)

File numpy/random/_generator.pyx:1288, in numpy.random._generator.Generator.normal()

File _common.pyx:621, in numpy.random._common.cont()

File _common.pyx:539, in numpy.random._common.cont_broadcast_2()

File _common.pyx:250, in numpy.random._common.validate_output_shape()

ValueError: Output size (1,) is not compatible with broadcast dimensions of inputs (2,).

Version In Use

main

Additional Context

- Python version: 3.11
- Operating system: macOS
@Saransh-cpp Saransh-cpp added the bug Something isn't working label Oct 4, 2024
@ntessore
Copy link
Collaborator

ntessore commented Oct 5, 2024

bad = np.where(eps > 0) ?

I see the problem. We probably need to broadcast_arrays() everything to the same shape, then select on all inputs. I don't see how there is another way in the general case.

Saransh-cpp added a commit that referenced this issue Oct 7, 2024
Instead of trying to reshape an array to a smaller dimension, produce an
array of the same `size` and mask it.

Fixes: #328 
Fixed: `ellipticity_ryden04` does not error our with shape mismatch
sporadically
Saransh-cpp added a commit that referenced this issue Oct 9, 2024
gh-328: efficient resampling in ellipticity_ryden04

Co-authored-by: Nicolas Tessore <[email protected]>
Saransh-cpp added a commit that referenced this issue Oct 9, 2024
gh-328: efficient resampling in ellipticity_ryden04

Co-authored-by: Nicolas Tessore <[email protected]>
Saransh-cpp added a commit that referenced this issue Oct 14, 2024
gh-328: efficient resampling in ellipticity_ryden04

Co-authored-by: Nicolas Tessore <[email protected]>
Saransh-cpp added a commit that referenced this issue Oct 14, 2024
The code now generates only the required values while resampling making
it more efficient.

Refs: #328
Changed: resampling in `ellipticity_ryden04` is more efficient now
Co-authored-by: Nicolas Tessore <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants