Update random_seed behavior in config to be less confusing. #1309
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rmjarvis
added
bug report
jmeyers314
approved these changes
Aug 19, 2024
| to be some complex Eval operation, you should make it a dict with an explicit | ||
| ``type: Eval``, rather than use the ``$`` shorthand notation. | ||
|
|
||
| * If ``random_seed`` is a list, then the first one will be converted as described above, but the later ones will not. Rather, it will respect your specification and evaluate the seed for each object as you specifiy. This will create multiple random number generators, according to the multiple seed specifications. This is normally used to have one random number behave normally for noise and such, and another one (or more) repeat with some other cadence (e.g. repeat for each image in an exposure to make sure you generate the same PSFs for multiple CCDs in an exposure). See `Demo 7` and `Demo 13` for examples of this. Whenever you want to use an rng other than the first one, add ``rng_num`` to the field and set it to the number of the rng you want to use in this list. |
There was a problem hiding this comment.
Maybe specify that rng_num starts at 0.
typo Co-authored-by: Josh Meyers <jmeyers314@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@kailicao discovered an error in the recent Roman image simulations where the random seed was not updating for each galaxy as expected. This only affected Poisson noise on the flux values (the "realized flux"), since most values were from SkyCatalog, not generated from an rng. But the statistics of the fluxes ended up messed up.
@matroxel and I tracked down the problem to the random_seed specification in the config file, which used a dict to evaluate a value based on the input file. This was intended to be the initial seed, to be converted into a sequence as usual. But the (documented) behavior in that case is to take the user's dict specification as is, and not convert it. This is confusing behavior, since most of the time evaluation like this is equivalent to setting a literal value.
Troxel also noticed that imsim's example config has the same problem, although the config files used by @jchiang87 for the recent simulation runs didn't use that. (It used a string eval value for the seed.) So imsim users may also be getting wrong random values currently.
So I changed the behavior to always convert the first random_seed in the list (or only one if not a list) into the sequence in the usual way. Any other items in the list are evaluated as the user specifies. This is technically an API change, since it's possible that some users have been relying on the old behavior to leave dict specifications as is and convert int or str values. However, (a) I think it's unlikely that anyone was using this feature other than to have the first item be normal and later items be dicts, and (b) we know of two accidental erroneous usages mentioned above, and it wouldn't surprise me if there were others. So this is more like a bug fix than a feature change. Anyway, I document it as an API change nonetheless to warn users who might be relying on the old behavior.