Enable refinement of lists and dictionary input parameters

[Luiskitsu]

I have modified the refine.py to allow refinement of scalars, lists, and dictionaries (before it was only scalars). I have done this by flattening lists and dictionaries accordingly and then reconstructing and regrouping the lists or dictionaries after refinement.
I have also added MorphSqueeze and MorphFuncy into morph_api.py and _init_.py. I have finally tested that we can now refine MorphSqueeze and MorphFuncy and I added these tests in test_morph_func.py where there are tests for other morphs as well. The tests are successful and therefore now we can refine squeeze and funcy. I am attaching some figures from the tests here. Finally, I also run all the tests for all the diffpy.morph, such as test_refine.py, etc., to make sure everything is working properly and they all passed.
Refinement test for scale + squeeze morph with same x-grid:

Refinement test for scale + squeeze morph with different x-grid:

Refinement test for MorphFuncy using a linear function that is applied to a sine wave:

Note that I also compare the refined parameters to the expected ones and is working well.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.23%. Comparing base (a450cd7) to head (a991072).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   97.11%   97.23%   +0.11%     
==========================================
  Files          20       20              
  Lines         866      903      +37     
==========================================
+ Hits          841      878      +37     
  Misses         25       25              

Files with missing lines	Coverage Δ
tests/test_morph_func.py	100.00% <100.00%> (ø)
tests/test_morphfuncy.py	100.00% <100.00%> (ø)
tests/test_morphsqueeze.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sbillinge]

I commented on the issues. Since you got this working I am inclinde to review it and merge it, but I would still like a discussion if this is the solution we want. If it is a workaround that needs a better fix we can create an issue and attach it to a future release maybe.

[sbillinge]

please see my comments. It looks really great as a general matter.

[sbillinge]

is there a reason we are not calling this funcy? or funcy_parameters? That seems to follow the previous pattern better.

[Luiskitsu]

I will change this to funcy to follow the pattern!

[sbillinge]

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.

[Luiskitsu]

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 modify squeeze and will be less elegant?

[Luiskitsu]

But I agree with you, from the user perspective it is better to be consistent and will be better to having all dicts

[Luiskitsu]

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 to

def _update_chain(self, pvals):
    """Update the parameters in the chain, supporting only scalars and dictionaries."""
    updated = {}
    for idx, value in enumerate(pvals):
        param, subkey = self.flat_to_grouped[idx]
        if subkey is None:
            # Scalar parameter
            updated[param] = value
        else:
            # Dictionary
            if param not in updated:
                updated[param] = {}
            updated[param][subkey] = value

    self.chain.config.update(updated)
    return

[sbillinge]

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.

[Luiskitsu]

Sounds good! So far, I’ve modified MorphSqueeze so that its parameter squeeze 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:

def _update_chain(self, pvals):
    """Update the parameters in the chain, assuming all parameters are dictionaries."""
    updated = {}
    for idx, value in enumerate(pvals):
        param, subkey = self.flat_to_grouped[idx]
        updated.setdefault(param, {})[subkey] = value
    self.chain.config.update(updated)
    return

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.

[Luiskitsu]

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

[sbillinge]

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.

[Luiskitsu]

If we just had scalars and dicts this would be simiplified to:

for p in self.pars:
    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)

[sbillinge]

no scalars, just dicts.

[sbillinge]

These tests look awesome!