gh-269: add tests for glass.fields #374
Conversation
Saransh-cpp
force-pushed
the
saransh/test_fields
branch
from
4563ff8
to
f4a047c
Compare
Saransh-cpp
force-pushed
the
saransh/test_fields
branch
from
f7a7090
to
e334b34
Compare
Saransh-cpp
force-pushed
the
saransh/test_fields
branch
from
9e5955d
to
f736b7a
Compare
I assume this is meant to say |
|
Should be ready now! I'll create a new issue for reviewing scientific tests for |
|
Almost at 80% with this PR |
Wow, that went up a lot! |
| assert gaussian_fields[0].shape == (hp.nside2npix(nside),) | ||
|
|
||
| # requires resetting the RNG for reproducibility | ||
| rng = np.random.default_rng(seed=42) |
There was a problem hiding this comment.
Why have it twice within this function?
There was a problem hiding this comment.
Should we set it once at the top of the file maybe?
There was a problem hiding this comment.
Need it twice to ensure that rng.any_distribution() is same both the times, and it needs to be redefined (instead of using rng from the arguments) so that it starts sampling from scratch.
Setting it at the top of the file gives me -
# requires resetting the RNG for reproducibility
> gaussian_fields = list(generate_gaussian(gls, nside, rng=rng))
E UnboundLocalError: cannot access local variable 'rng' where it is not associated with a valueThere was a problem hiding this comment.
Okay! Might a little comment saying why it's being redefined?
There was a problem hiding this comment.
I think the right approach here is to use our central rng fixture and clone it:
from copy import deepcopy
def test_xyz(rng):
rng1 = deepcopy(rng)
rng2 = deepcopy(rng)
...There was a problem hiding this comment.
This probably needs a fairly recent numpy
There was a problem hiding this comment.
But what is this actually testing? If it's that the generation is reproducible, it's on the RNG, not on us. In which case I would rather test that the rng was actually used. If you don't want to mock the entire RNG interface (let's not), we can check that the state changed:
initial_rng_state = rng.bit_generator.state
...
assert rng.bit_generator.state != initial_rng_stateThere was a problem hiding this comment.
My initial thought behind adding this test was to check if generate_gaussian with and without ncorr=1 outputs the same result (because that is a pattern that I observed while running the function manually). Is there a better test that can be added to check if passing the ncorr argument works, or should I leave it for #414?
| rng = np.random.default_rng(seed=42) | ||
| lognormal_fields = list(generate_lognormal(gls, nside, shift=1.0, rng=rng)) | ||
|
|
||
| rng = np.random.default_rng(seed=42) |
Add tests and bump coverage to ~100 for
glass.fields.Closes: #269