-
Notifications
You must be signed in to change notification settings - Fork 18
Enable refinement of lists and dictionary input parameters #190
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
Changes from all commits
40ecd00
82848b4
e198331
997c8c1
ca832a6
655676e
a991072
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
**Added:** | ||
|
||
* Functionality for refining lists and dictionaries | ||
|
||
**Changed:** | ||
|
||
* <news item> | ||
|
||
**Deprecated:** | ||
|
||
* <news item> | ||
|
||
**Removed:** | ||
|
||
* <news item> | ||
|
||
**Fixed:** | ||
|
||
* <news item> | ||
|
||
**Security:** | ||
|
||
* <news item> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,12 +51,23 @@ def __init__(self, chain, x_morph, y_morph, x_target, y_target): | |
self.y_target = y_target | ||
self.pars = [] | ||
self.residual = self._residual | ||
self.flat_to_grouped = {} | ||
return | ||
|
||
def _update_chain(self, pvals): | ||
"""Update the parameters in the chain.""" | ||
pairs = zip(self.pars, pvals) | ||
self.chain.config.update(pairs) | ||
updated = {} | ||
for idx, value in enumerate(pvals): | ||
param, subkey = self.flat_to_grouped[idx] | ||
if subkey is None: # Scalar | ||
updated[param] = value | ||
else: | ||
if param not in updated: | ||
updated[param] = {} | ||
updated[param][subkey] = value | ||
|
||
# Apply the reconstructed grouped parameter back to config | ||
self.chain.config.update(updated) | ||
return | ||
|
||
def _residual(self, pvals): | ||
|
@@ -118,11 +129,25 @@ def refine(self, *args, **kw): | |
if not self.pars: | ||
return 0.0 | ||
|
||
initial = [config[p] for p in self.pars] | ||
# Build flat list of initial parameters and flat_to_grouped mapping | ||
initial = [] | ||
self.flat_to_grouped = {} | ||
|
||
for p in self.pars: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my suggested refactor probably gets rid of this too....so there is a lot of technical debt we remove with this refactor if we decide to do it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we just had scalars and dicts this would be simiplified to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no scalars, just dicts. |
||
val = config[p] | ||
if isinstance(val, dict): | ||
for k, v in val.items(): | ||
initial.append(v) | ||
self.flat_to_grouped[len(initial) - 1] = (p, k) | ||
else: | ||
initial.append(val) | ||
self.flat_to_grouped[len(initial) - 1] = (p, None) | ||
|
||
sol, cov_sol, infodict, emesg, ier = leastsq( | ||
self.residual, initial, full_output=1 | ||
) | ||
fvec = infodict["fvec"] | ||
|
||
if ier not in (1, 2, 3, 4): | ||
emesg | ||
raise ValueError(emesg) | ||
|
@@ -131,7 +156,7 @@ def refine(self, *args, **kw): | |
vals = sol | ||
if not hasattr(vals, "__iter__"): | ||
vals = [vals] | ||
self.chain.config.update(zip(self.pars, vals)) | ||
self._update_chain(vals) | ||
|
||
return dot(fvec, fvec) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,3 +101,58 @@ def test_smear_with_morph_func(): | |
assert np.allclose(y0, y1, atol=1e-3) # numerical error -> 1e-4 | ||
# verify morphed param | ||
assert np.allclose(smear, morphed_cfg["smear"], atol=1e-1) | ||
|
||
|
||
def test_squeeze_with_morph_func(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests look awesome! |
||
squeeze_init = {"a0": 0, "a1": -0.001, "a2": -0.0001, "a3": 0.0001} | ||
x_morph = np.linspace(0, 10, 101) | ||
y_morph = 2 * np.sin( | ||
x_morph + x_morph * 0.01 + 0.0001 * x_morph**2 + 0.001 * x_morph**3 | ||
) | ||
expected_squeeze = {"a0": 0, "a1": 0.01, "a2": 0.0001, "a3": 0.001} | ||
expected_scale = 1 / 2 | ||
x_target = np.linspace(0, 10, 101) | ||
y_target = np.sin(x_target) | ||
cfg = morph_default_config(scale=1.1, squeeze=squeeze_init) | ||
morph_rv = morph(x_morph, y_morph, x_target, y_target, **cfg) | ||
morphed_cfg = morph_rv["morphed_config"] | ||
x_morph_out, y_morph_out, x_target_out, y_target_out = morph_rv[ | ||
"morph_chain" | ||
].xyallout | ||
assert np.allclose(x_morph_out, x_target_out) | ||
assert np.allclose(y_morph_out, y_target_out, atol=1e-6) | ||
assert np.allclose( | ||
expected_squeeze["a0"], morphed_cfg["squeeze"]["a0"], atol=1e-6 | ||
) | ||
assert np.allclose( | ||
expected_squeeze["a1"], morphed_cfg["squeeze"]["a1"], atol=1e-6 | ||
) | ||
assert np.allclose( | ||
expected_squeeze["a2"], morphed_cfg["squeeze"]["a2"], atol=1e-6 | ||
) | ||
assert np.allclose( | ||
expected_squeeze["a3"], morphed_cfg["squeeze"]["a3"], atol=1e-6 | ||
) | ||
assert np.allclose(expected_scale, morphed_cfg["scale"], atol=1e-6) | ||
|
||
|
||
def test_funcy_with_morph_func(): | ||
def linear_function(x, y, scale, offset): | ||
return (scale * x) * y + offset | ||
|
||
x_morph = np.linspace(0, 10, 101) | ||
y_morph = np.sin(x_morph) | ||
x_target = x_morph.copy() | ||
y_target = np.sin(x_target) * 2 * x_target + 0.4 | ||
cfg = morph_default_config(funcy={"scale": 1.2, "offset": 0.1}) | ||
cfg["function"] = linear_function | ||
morph_rv = morph(x_morph, y_morph, x_target, y_target, **cfg) | ||
morphed_cfg = morph_rv["morphed_config"] | ||
x_morph_out, y_morph_out, x_target_out, y_target_out = morph_rv[ | ||
"morph_chain" | ||
].xyallout | ||
assert np.allclose(x_morph_out, x_target_out) | ||
assert np.allclose(y_morph_out, y_target_out, atol=1e-6) | ||
fitted_parameters = morphed_cfg["funcy"] | ||
assert np.allclose(fitted_parameters["scale"], 2, atol=1e-6) | ||
assert np.allclose(fitted_parameters["offset"], 0.4, atol=1e-6) |
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 bit hard to follow. I think the code is fine, I just wonder if there is a more elegant and readable way of doing this.
I guess the basic structure "before" was that we had two ordered lists of equal length containing parameter names(?) and values. Apart from this structure being a bit brittle and possibly not optimal, but ok, np. I guess the task here then is that we introduce parameters that are not singles but can be, themselves lists or dicts. If they are sometimes lists and sometimes dicts are shooting ourselves in the foot because we actually control this ourselves, so we could maybe decide on what the best structure is and then stick to that. If we pick a dict, which seems to be the most logical, then there is no different between singles, doubles etc.. So I suggest that we think about this, but maybe change the basic type of our "parameter" objects. I would vote for dicts.
to be clear, I am ok parsing them out into zipped lists for passing to other parts of the code if that is what they are expecting, but just storing them in the Morph object as a dict seems to me to make the most sense.
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.
If I understood your comment correctly, you're proposing to standardize all morph parameters to use dictionaries — is that across the board, or only for lists (e.g.,
squeeze
) while keeping scalar values as-is? I agree that moving to dictionaries would improve consistency.The only downside I see is that users would need to explicitly define each coefficient in the squeeze parameter like:
squeeze = {"a0": 0.1, "a1": -0.01, "a2": 0.005}
We'd then need to convert this dict into a list internally before passing it to
Polynomial(squeeze)
, which expects a list of coefficients. My point is that we can make the refinement code more elegant by having all inputs as dicts but then we probably will have to modifysqueeze
and will be less elegant?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.
But I agree with you, from the user perspective it is better to be consistent and will be better to having all dicts
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.
If we want to have squeeze as a list I can do the corresponding modifications to the code and the tests. The
refine.py
will be simplified toThere 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.
internals of the code and user experience are two different things. We can wrap anything. The basic conversation is "what is the best type for the basic parameter data object?" We would often define a container class to carry these things around, something like
class MorphParameters
but since it is basically a set we could just use a dictionary. There would be no "scalar" parameter. A scalar parameter would be a dictionary with one key.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.
Sounds good! So far, I’ve modified
MorphSqueeze
so that its parametersqueeze
is now represented as a dictionary—for example:squeeze = {"a0":0, "a1":0.01, "a2":0.0001, "a3":0.001}
If we want to generalize this approach, I could go ahead and update all the morphs, tests and other functions, so that all parameters that are scalars are stored as dictionaries—for instance:
scale = {"scale": 0.1}
With this change the refine code will be reduced to:
However, I’m also wondering if it's worth preserving scalars as-is for simplicity and only using dictionaries where multiple sub-parameters are required. That would avoid the somewhat inelegant repetition of having to write
scale = {"scale": 0.1}
.Happy to implement either approach. We can also discuss this tomorrow and choose what direction we want to take.
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.
What if we keep scalars and have scalars and dictionaries and create an issue for a near future? A lot of the code that was already written for the other morphs and chains will need to be modified, so I am not sure how quick of a fix is this