Lattice kronecker - #556
Conversation
…into kronecker_search
…e rule search method
…Software into lattice_kronecker
…into lattice_kronecker
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #556 +/- ##
============================================
- Coverage 88.16% 35.89% -52.28%
============================================
Files 100 103 +3
Lines 8081 8332 +251
============================================
- Hits 7125 2991 -4134
- Misses 956 5341 +4385
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
alegresor
left a comment
alegresor
left a comment
There was a problem hiding this comment.
Hi @AndersPride, thanks for submitting this PR, it looks very technically sound! Some suggestions are below
- I think it is fine to leave your
kroneckerr_search_methods.pyandlattice_vector_wssd_search.pyfiles where they are for now - Try to reuse similar parameter names in your functions as used in other discrete distribution classes
- Make sure all your functions have docstrings and doctests
- For the
lattice_kronecker_methods.ipynbdemo, is thekuo.lattice-39102...txtfile in the LDData repo? If so, then you can construct a lattice by passing in the LDData path, see the lattice doctests. Then you can also avoid explicitly saivng this .txt file. - Also in the demo, it would be nice to see plots comparing the standard lattice / Kronecker points compared to your new and improved constructions
| @@ -0,0 +1,179 @@ | |||
| import numpy as np | |||
| from sympy import gcdex, primerange, prime | |||
There was a problem hiding this comment.
sympy is not included in the default requirements for qmcpy. Please import sympy with the functions where it is called so it will not throw an error when users try to import qmcpy without having sympy installed.
| @@ -363,7 +474,10 @@ def wssd_discrepancy(self, n, weights, k_tilde = None, gamma = None): | |||
|
|
|||
| def _square_periodic_discrepancies(self, n, k_tilde, gamma): | |||
There was a problem hiding this comment.
Maybe this makes more sense as a function in kronecker_search_methods.py?
There was a problem hiding this comment.
I'm not sure if it does or not, since it is used as a method to calculate discrepancy for any Kronecker object, as opposed to being solely focused on search methods.
There was a problem hiding this comment.
Let's leave it here for now and we can always move it in the future
|
@AndersPride The "All Tests" failures can probably be resolved by merging |
There was a problem hiding this comment.
Is this file available from LDData? Do we need to duplicate it here?
There was a problem hiding this comment.
This is not available in LDData, but it would be nice to add it!
There was a problem hiding this comment.
Is this file available from LDData? Do we need to duplicate it here?
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Changes I have made since @fjhickernell merged |
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Changed variable & function names to be more consistent and descriptive. Fixed imports where needed and removed a few extraneous lines. Added docstrings/doctests where needed. Updated examples in the lattice_kronecker_methods demo to ensure a reasonable run time.
Changed == None to is None Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
|
@AndersPride @fjhickernell @alegresor Some doc or unit tests were failing. I have made some minimal changes to address them. Please review them here |
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: sou-cheng-choi <18743024+sou-cheng-choi@users.noreply.github.com>
Resolved by merging |
There was a problem hiding this comment.
Pull request overview
Adds generator search utilities and new/updated discrepancy + WSSD computations for Lattice and Kronecker sequences, along with tests, docs, and demo assets to illustrate and validate the new functionality.
Changes:
- Add Lattice expected squared periodic discrepancy and WSSD APIs, plus a CBC-style WSSD-based lattice generating-vector search.
- Add Kronecker Mobius-transform CBC search utility and a new built-in
CBC_MTgenerating vector option. - Add unit tests, booktest coverage for a new demo notebook, and API documentation entries.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_dd_lattice_kronecker.py | Adds unit tests covering the new discrepancy/WSSD APIs and both search methods. |
| test/booktests/tb_lattice_kronecker_methods.py | Adds a notebook regression test with downscaled parameters for CI runtime. |
| qmcpy/discrete_distribution/lattice/lattice.py | Introduces expected_squared_periodic_discrepancies and wssd on Lattice. |
| qmcpy/discrete_distribution/lattice/lattice_vector_wssd_search.py | Adds CBC search routine for a lattice generating vector minimizing WSSD. |
| qmcpy/discrete_distribution/lattice/init.py | Exports lattice_vector_wssd_search from the lattice package. |
| qmcpy/discrete_distribution/kronecker/kronecker.py | Adds "CBC_MT" generating-vector option and updates discrepancy/WSSD internals. |
| qmcpy/discrete_distribution/kronecker/kronecker_search_methods.py | Adds Mobius-transform-based Kronecker generator search method. |
| qmcpy/discrete_distribution/kronecker/generating_vectors/kron_vector_d-100_N-2exp20_2026_06_01.txt | Adds a 100D precomputed Kronecker generating vector data file. |
| qmcpy/discrete_distribution/kronecker/init.py | Exports Kronecker and the new search function from the kronecker package. |
| qmcpy/discrete_distribution/init.py | Re-exports the new lattice/kronecker search functions at qmcpy.discrete_distribution (and thus qmcpy). |
| docs/api/discrete_distributions.md | Documents the new public search utilities in the API docs. |
| demos/kuo.lattice-39102-1024-1048576.3600.txt | Adds a large demo data asset (Kuo lattice data). |
Suppressed comments (3)
qmcpy/discrete_distribution/kronecker/kronecker.py:481
pointsis built with shape(n, 1, d), which makesk_tilde_termshave length 1 on the last axis; subsequent slicingk_tilde_terms[..., 1:]then becomes empty and the discrepancy computation breaks. The sample index axis should be the last axis (shape(r, n, d)before applyingk_tilde).
qmcpy/discrete_distribution/kronecker/kronecker_search_methods.py:52sympymay be unavailable (it is not a core dependency). This code should fail fast with an explicit error message rather than raising anAttributeErrorwhensympyisNone.
# search over the first n primes, n = searchsize
searchspace = np.array(list(sympy.primerange(1, sympy.prime(searchsize)+1)), dtype=np.float64)
qmcpy/discrete_distribution/kronecker/kronecker_search_methods.py:60
np.moddoes not accept adtype=argument; this line will raiseTypeErrorwhengen_vec_initis provided.
else:
gen_vec[0] = np.mod(gen_vec_init, 1,dtype=np.float64)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if sample_weights is not None and len(sample_weights) < n_max: | ||
| raise ValueError("Length of sample_weights must be equal to n_max") |
| # get the frequency matrix for how often each kernel evaluation appears (this is always the same and can be precomputed, not done here to avoid adding >1GB txt file to git) | ||
| freq_mtx = np.zeros((k_sum.size, n_max), dtype=np.float64) | ||
| for i in range(1,n_max): | ||
| for j in range(k_sum.size): | ||
| if np.floor(i / 2**j) % 2 == 1: |
There was a problem hiding this comment.
As I mentioned in the comment on line 443, this matrix is always the same, and therefore can be precomputed and stored for reference. However, it has (n log n) values, so the file can be quite large. @alegresor @fjhickernell, what would you say is the best approach here?
There was a problem hiding this comment.
I approve this PR. Thank you for your contribution.
- It seems there are a few conflicts with
developbranch. Please resolve them. - Copilot has made some suggestions. Please evaluate if they are valid.
- Please ask @alegresor and @fjhickernell for re-reviews and address their feedback if any before you merge in the work.
There was a problem hiding this comment.
Looks like it's in pretty good shape! A few minor suggestions
- You have introduced three new generating vectors, the
kuo.lattice-39102...txt, thekron_vector_d-100...txt, and theCBCMT = np.array([0.618033988749895,...]). These would all be nice to have in our sisterQMCSoftware/LDDatarepo. I am fine if you want to leave this to a separate PR as it will take some time to spell out new formats for Kronecker vectors in LDData as they will be the first of their kind! - Add
kron_vector_d-100...txtto thepyproject.tomlincludes block so it gets included when userspip install qmcpy. - Do not
import sympyat the top level. Only import exactly where it is needed within the function.sympyis not a requirement forqmcpy, so having it imported at the top level would break things for users. Importing within the function it is used only throws errors when users call that function withoutsympy. - Address merge conflicts
- Copilot has some other good suggestions.
As we want to finalize QMCPy v2.4 PRs tonight, I think this will be a nice feature for QMCPy v2.5. Please re-request a review from me when you have addressed these small changes, and make sure to also get approval from @fjhickernell before merging.
There was a problem hiding this comment.
This is not available in LDData, but it would be nice to add it!
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Improved docstring consistency Fixed two instances where a previous approach was not properly updated
…CSoftware into lattice_kronecker
- Adds exception handling for import sympy. Prompts user to install sympy or choose to use the slower, recursive implementation instead. - Removes kuo.lattice-39102-1024-1048576.3600.txt file from demos and replaces it with a .npy file in the lattice\generating_vectors folder, like the other Kuo vector. Also edits lattice.py to accommodate this. Changes the demo notebook to use this vector, fairer comparison. - Adds the kronecker vector .txt file to pyproject.toml so that it is included in the package.
Adds search methods for generators for lattice and kronecker sequences. Also adds and updates methods for finding discrepancies and weighted sum of squared discrepancies for arbitrary sample size, dimension, and coordinate weights. Currently the search methods live in their respective folders in discrete_distribution, but they may belong elsewhere. There is a new demo illustrating these features. Right now, this is not integrated into the kernel class, but that can be future work.