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

fix(noise): RandomGenerator rename and other GaussianNoiseDataset bugs #106

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

ssiegelx
Copy link
Contributor

@ssiegelx ssiegelx commented Oct 2, 2020

  • randomgen 1.18 renamed RandomGenerator to Generator.
    This commit renames Generator back to RandomGenerator on import
    for synthesis.noise, in the same way as was done in analysis.delay.
  • Fixes bug in how autocorrelations are being identified in GaussianNoiseDataset
  • Fixes inconsistent variable name in mpi_random_seed
    (both 'gen' and 'randomgen' are used to refer to the same variable)

- randomgen 1.18 renamed RandomGenerator to Generator.
This commit renames Generator back to RandomGenerator on import
for synthesis.noise, in the same way as was done in analysis.delay.
- Fixes bug in how autocorrelations are being identified in GaussianNoiseDataset
- Fixes inconsistent variable name in mpi_random_seed
(both 'gen' and 'randomgen' are used to refer to the same variable)
@ssiegelx
Copy link
Contributor Author

ssiegelx commented Oct 2, 2020

This applies the same fix to synthesis.noise as was applied to analysis.delay
regarding randomgen version 1.18 renaming RandomGenerator to Generator.

Note that currently randomgen is spamming the logs with this warning message:

707.6s [MPI 66/88] - WARNING  py.warnings: /home/ssiegel/ch_pipeline_rev01/venv/src/draco/draco/synthesis/noise.py:106: FutureWarning: Generator is deprecated and will be removed sometime after the release of
NumPy 1.21 (or 2 releases after 1.19 if there is a major release).

Unique features of Generator have been moved to
randomgen.generator.ExtendedGenerator.

Now is the time to start using numpy.random.Generator.

In the mean time Generator will only be updated for the most egregious bugs.

You can silence this warning using

import warnings
warnings.filterwarnings("ignore", "Generator", FutureWarning)

rg = randomgen.generator.Generator()

So perhaps we should modify this PR to use ExtendedGenerator or numpy?
It looks like ExtendedGenerator adds several distributions not included
in numpy, specifically complex_normal, multivariate_normal, and uintegers.
I don't think these distributions are being used at the moment, although
I could imagine using something like complex_normal in the future.

@jrs65
Copy link
Contributor

jrs65 commented Oct 2, 2020

I just started changing some of this in my PR as well, but let's merge what you have here, and I'll rebase on top.

It seems like we should just drop randomgen entirely in favour of numpy. We'd need to bump the minimum version up to 1.17, but I think that's fine. If you agree, I'll do that in my next PR.

@anjakefala
Copy link
Contributor

@jrs65 (for your next PR: #56)

@ssiegelx
Copy link
Contributor Author

ssiegelx commented Oct 2, 2020

@jrs65 Okay sounds good. I'm fine with moving to numpy.

@ssiegelx ssiegelx merged commit ba7648d into master Oct 2, 2020
@ssiegelx ssiegelx deleted the ss/gen_noise branch October 2, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants