Conversation
Adapt sample-function accordingly. Adapt qmc-test accordingly.
FriesischScott
left a comment
There was a problem hiding this comment.
Types shouldn't be abbreviations. Since QuasiMonteCarlo is already the package name that needs to be loaded I think going with QuasiMonteCarloSampling is probably a safe choice.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Change simulations.md
fix small problems in docs
add further qmc test
There was a problem hiding this comment.
🟡 Changes recommended
Singleton dispatch regressions, unmigrated legacy references, stale exports, and incompatible lower-bound dependency support remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces algorithm-specific quasi-Monte Carlo wrappers with one configurable QuasiMonteCarloSampling wrapper and updates dependent tests and documentation.
Changes:
- Adds the unified wrapper and sample-doubling support.
- Migrates tests and examples to QuasiMonteCarlo.jl samplers.
- Expands QMC documentation and references.
File summaries
| File | Description |
|---|---|
src/UncertaintyQuantification.jl |
Exports the new wrapper and helper. |
src/simulations/montecarlo.jl |
Implements unified QMC sampling. |
test/Project.toml |
Adds the QMC test dependency. |
test/simulations/montecarlo.jl |
Tests the unified wrapper. |
test/sensitivity/sobolindices.jl |
Migrates sensitivity tests. |
test/reliability/probabilityoffailure_imprecise.jl |
Updates QMC-related comments. |
test/models/responsesurface.jl |
Migrates response-surface sampling. |
test/models/pce/polynomialchaosexpansion.jl |
Migrates PCE sampling. |
test/models/basisfunctionmodels.jl |
Migrates basis-model sampling. |
docs/Project.toml |
Adds the QMC documentation dependency. |
docs/src/manual/simulations.md |
Rewrites QMC guidance and examples. |
docs/src/manual/metamodels.md |
Migrates metamodel examples. |
docs/src/manual/gettingstarted.md |
Updates introductory examples. |
docs/src/api/simulations.md |
Removes the obsolete Halton entry. |
docs/references.bib |
Adds supporting QMC references. |
Review details
Suppressed comments (1)
docs/src/manual/simulations.md:59
- Correct the spelling of “choosing.”
When chosing `n`, be reminded that, for digital nets, `n` must be a power of the base that is used for creating the respective sequence. For `SobolSample` the base is always equal to 2 while for `FaureSample`, it depends on the number of input-variables, being the smallest prime number that is greater or equal to the number of variables.
- Files reviewed: 15/15 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| end | ||
|
|
||
| struct SobolSampling <: AbstractQuasiMonteCarlo | ||
| struct QuasiMonteCarloSampling <: AbstractQuasiMonteCarlo |
There was a problem hiding this comment.
@mlsuh Since we now only have one QMC struct we don't need the AbstractQuasiMonteCarlo type anymore. We can directly dispatch on QuasiMonteCarloSampling.
| export TransportMapBayesian | ||
| export TwoLevelFactorial | ||
| export UQTargetDensity | ||
| export QuasiMonteCarloSampling |
There was a problem hiding this comment.
Please remove the undefined exports.
| sobol = QuasiMonteCarloSampling( | ||
| 64, SobolSample(R = OwenScramble(base = 2, pad = 32)) | ||
| ) |
There was a problem hiding this comment.
I think we can drop QuasiMonteCarlo v0.3
There was a problem hiding this comment.
Completely dropping v0.3 is not possible currently, beacuase TransportMaps.jl does not allow v0.4 yet. I have tested the flagged syntax with v0.3.11 and it works without problems. I have specified the compatability to v0.3.11 and v0.4.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
FriesischScott
left a comment
There was a problem hiding this comment.
Just a few minor things left to fix.
| export TransportMapBayesian | ||
| export TwoLevelFactorial | ||
| export UQTargetDensity | ||
| export QuasiMonteCarloSampling |
There was a problem hiding this comment.
Please remove the undefined exports.
| sobol = QuasiMonteCarloSampling( | ||
| 64, SobolSample(R = OwenScramble(base = 2, pad = 32)) | ||
| ) |
There was a problem hiding this comment.
I think we can drop QuasiMonteCarlo v0.3
also, revise QMC sample-sizes in demos and docs in order not to promote suboptimal usage
AnderGray
left a comment
There was a problem hiding this comment.
Small comment wrt the remaining unresolved joint distribution issue
This PR introduces a single struct called QMC that serves as a wrapper for sampling-methods from QuasiMonteCarlo.jl.
This simplifies our current structure where there is a struct per sampling-algorithm, all of which wrap the corresponding QuasiMonteCarlo.jl struct.
I have also changed the corresponding tests and adapted all other tests that use QMC-methods.
I still need to update the documentation.
closes #327