-
Notifications
You must be signed in to change notification settings - Fork 65
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
Temperature feature #552
base: master
Are you sure you want to change the base?
Temperature feature #552
Conversation
…ormal tests). Add support for 2-parameter estimation in context of background susceptibility (WIP). PEP8 Compliance changes.
@LoganLim2271 has offered to review. |
Can't reproduce these failing tests in a new python 3.13 environment. The errors relate to SciPy return object, but scipy 1.15 (default install) and mos trecent 2.2.1 both throw no errors for me. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #552 +/- ##
==========================================
- Coverage 88.75% 85.81% -2.94%
==========================================
Files 21 21
Lines 1654 1777 +123
==========================================
+ Hits 1468 1525 +57
- Misses 186 252 +66 ☔ View full report in Codecov by Sentry. |
@kevinchern is also willing to take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have to look at this intermittently, some minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some to-dos
- typeset: math, URLs, variable/code name
- complete docstrings, e.g., args and return types
What are the motivation for the many customizable arguments? For example,
- passing in
degenerative_fields
- optimization method (is there a robust method)
- univariate case versus calling multivariate case with, e.g., f(x)->f([x])
Optimize method used by SciPy ``root_scalar`` method. The default | ||
method works well under default operation, 'bisect' can be | ||
numerically more stable for the scalar case (Temperature estimation | ||
only. | ||
T_bracket (list or Tuple of 2 floats, optional, default=(0.001,1000)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default excludes 0.
Should note the following in the docstring:
- The arbitrary choice of
0.001
and - There is a check for when
T=0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't the optimization methods in scipy return boundary values? i.e., no need to check for T=0
and leave it to scipy to return 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't trust scipy to handle the case beta=Infinity (T=0) correctly. It turns out this is a very common outcome since the main usage is for heuristic optimizers that return local minima exclusively (or disproportionately).
method works well under default operation, 'bisect' can be | ||
numerically more stable for the scalar case (Temperature estimation | ||
only). | ||
bisect_bracket: Relevant only if optimize_method='bisect' and for a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above: method-specific args could be packed into a kwargs_opt=None
arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion. For legacy reasons I didn't think of it (was originally matlab translated to python within sciPy dependence).
): | ||
# Only local minima (or maxima) observed | ||
x = np.sign(max_excitation) * float("Inf") | ||
if max_excitation == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document this special case
# Infinite results from all local minima, well defined limit | ||
x_bootstraps = x * np.ones(num_bootstrap_samples) | ||
else: | ||
# To do: Add to examples! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
return -1 / x, -1 / x_bs | ||
|
||
|
||
def maximum_pseudolikelihood( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has become a very long function with many branches. Could it be split up and organized into smaller functions for the various cases and checks? That would make it much easier to review and maintain, and would also allow for better documentation and comments.
… generic dimod.Samplers(). Add clarifying documentation.
|
Add generators for the background susceptibility correction to binary quadratic models.
Generalize the maximum_pseudolikelihood estimator for multidimensional parameter inference: use cases include estimation of the background susceptibility, estimation of a conversion factor from flux biases to (unitless) linear fields, estimating relative rescaling of h at freezeout when using h_gain.
Add type hinting, improve documentation.
Standardize formatting (black) on temperature.py and related tests.