Let the sampling of the field and the pupil be seeded - #220
Merged
Conversation
The field and the pupil are sampled at a point drawn inside each cell rather than at its center, which is what keeps the quadrature from aliasing against an edge that falls between samples. Nothing could fix that sampling, so nothing computed through it could be reproduced. Two calls to `area_effective` on the ESIS model, with nothing changed between them, gave effective areas about two percent apart, and an article which draws that curve drew a different one every time it was built. `area_effective` takes a seed, as does the sampling behind `image`, whose noise comes from the same draw. The field and the pupil are drawn from two streams derived from that one seed rather than from the seed itself. A seed shared between them would offset a field cell and a pupil cell by the same fraction wherever the two grids agreed in shape, which is not a sample of anything. The default is to draw as before, so nothing computed today moves. A caller who wants an answer twice now has a way of asking for it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 121 121
Lines 7491 7500 +9
=========================================
+ Hits 7491 7500 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or 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
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.
area_effectivesamples the field and the pupil at a point drawn inside each cell rather than at its center, which is what #219 established: it keeps the quadrature from aliasing against an edge which falls between samples. Nothing could fix that sampling, so nothing computed through it could be reproduced.Two calls on the ESIS model, nothing changed between them:
About two percent apart, which is the same size as the effect #219 was measured against. An article which draws that curve draws a different one every time it is built, and a count rate computed from it moves with it.
With a seed
Two streams, not one
The field and the pupil are drawn from two seeds derived from the given one with
SeedSequence, rather than both from the seed itself. Sharing it would offset a field cell and a pupil cell by the same fraction wherever the two grids agreed in shape — the same failure as giving every axis of one grid the same offset, which puts each sample on the diagonal of its cell instead of inside it.named-arraysguards the second case; this guards the first.The default
None, so the sampling draws as it does today and nothing computed now moves.The alternative is a fixed default, which would make every result reproducible without being asked. That is arguably what this should be — the randomness is a quadrature technique rather than anything physical, and a user who wants a different realization can say so. It would shift every existing result once, by about the two percent above, so I have not made that choice here. Worth deciding deliberately.
Requires
named-arrays~=2.9, which added theseedparameter tocell_centers(sun-data/named-arrays#230).