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

[WIP] WLS_sparse2 solver #121

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

bdestombe
Copy link
Collaborator

@bdestombe bdestombe commented Jun 5, 2020

Introduces a new solver that accepts prior knowledge of the parameter set. And should generally be faster than wls_spare.

Solves the normal equations of X . p_sol = y with weights. Supports a priori
information. The parameter estimates from a previous calibration instance
and their covariances can be passed to `p_sol_prior` and `p_cov_prior`.
Allows for sequential calibration in chunks.

Normally, the X matrix is very tall (nobs>>npar), more observations than
unknowns. By using the normal equations, the coefficient matrix reduces to a
square matrix of shape (npar, npar), and the observation matrix to (npar,).
This results in a much smaller system to solve.

Todo:

  • Test / demo that sequential calibration works
  • Similar tests as the other solvers
  • Basic implementation in the ds.calibration_single_ended() and ds.calibration_double_ended() functions.
  • Example notebook

Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
…ntial calibration in chunks.

Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
@bdestombe
Copy link
Collaborator Author

#110

Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

Merging #121 into master will decrease coverage by 0.38%.
The diff coverage is 58.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   75.25%   74.86%   -0.39%     
==========================================
  Files           8        8              
  Lines        3180     3258      +78     
  Branches      687      713      +26     
==========================================
+ Hits         2393     2439      +46     
- Misses        585      610      +25     
- Partials      202      209       +7     
Impacted Files Coverage Δ
src/dtscalibration/datastore.py 75.72% <7.69%> (-0.59%) ⬇️
src/dtscalibration/calibrate_utils.py 84.46% <69.23%> (-1.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 139f0e4...4e2b5b5. Read the comment docs.

Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
@BSchilperoort
Copy link
Collaborator

Nice work!

One question though; is sparse2 an appropriate name? I think something more self-descriptive might be better for clarity. Or is the plan that this solver supersedes the old one?

@bdestombe
Copy link
Collaborator Author

I would think the latter. There would be no need for two sparse solvers so wls_sparse2 would supersede wls_sparse, at least I can't come up with a reason to keep both. We should keep the statsmodels version for testing purposes.

It is going to be a pain to fully implement this though in ds.calibration_single_ended() and ds.calibration_double_ended() and keep on support all those fixed parameter options..

@bdestombe
Copy link
Collaborator Author

But I'm open to using other names :) we could drop the wls_ since all implemented solvers support weights.

@BSchilperoort
Copy link
Collaborator

wls_sparse2 is fine as a temporary name as it's going to replace the old one. For continuity sake I think we have to keep the name 'wls_sparse' though, to avoid breaking people's code (which is something we have to worry about now we're at version 1.0).

Fixing parameters is a mess yes, but extremely useful! Would there be a cleaner way to handle it in the back-end? Also wrap it in one or multiple functions perhaps?

@bdestombe
Copy link
Collaborator Author

For now, I think the path forward would be to:

  1. Invoke calibration_single_ended_solver() and calibration_double_ended_solver() directly with solver='external' for each calibration instance. It returns for each instance the matrices needed for calibration (X, y, w, p0_est).
  2. Manually stretch X, p0_est to account for the parameters that are in the first calibration instance but not in the second.
  3. Call wls_sparse2 directly to compute p_sol, p_cov for each calibration instance.
  4. Invoke ds.calibration_single_ended() and ds.calibration_double_ended() with solver='external' and pass the p_sol, p_cov from the final calibration instance as arguments.

Thus quite some manual labor, but full control.. And If you'd like to fix certain parameters, you could use solver='external_split' so you retrieve an X, y, w, p0_est for every parameter separately.

Maybe there should be two example notebooks. 1. Without fixing any parameters and 2. With fixing gamma.

Signed-off-by: Bas des Tombe <bdestombe@gmail.com>
@bdestombe bdestombe marked this pull request as draft July 31, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants