Yeah, it might be worth putting this back in for at least Parametric models.

The problem is that there's ambiguity in how multiple parameter sets are currently assigned (without explicitly giving param_dim). This is because parameter values don't have to be scalars. For example, you can have param_dim=1 but still use a value like [1, 2] for the parameter (in the case of Const1D representing a constant vector).

But in most cases I agree that if param_dim isn't specified, and that the amplitude is some 1xn array, then param_dim=n is a sensible default. That will hold for most models currently implemented, but not necessarily all that may be implemented so this needs to be given more thought I think. But for the short term I can restore the previous behavior on this.

Related to this issue, the following is also wrong:

model = models.PowerLaw1DModel([10, 2], [1, 3], [2, 2])
model.param_dim
Out[13]: 1

@embray - I am confused. How can 1D model accept a vector? Vector is 2D.

"1D" may be a bit of a misnomer in this case, as it refers to the number of arguments to the function (in this case f(x) = C). A vector can be n-dimensional... (and when I wrote vector I really meant array in this case since all operators on Numpy arrays are just applied element-wise, though one would generally use an array to represent a vector).

Maybe we are thinking about different definition of vectors?

http://en.wikipedia.org/wiki/Euclidean_vector

So what is the behavior of Const1DModel([x1, x2], param_dim=1)?

The difference is basically in the "shape" of the parameter value:

In [6]: c1 = models.Const1DModel([1, 2], param_dim=1)

In [7]: c1.amplitude.shape
Out[7]: (2,)

In [8]: c2 = models.Const1DModel([1, 2], param_dim=2)

In [9]: c2.amplitude.shape
Out[9]: ()

I agree though this is still a little confusing, though this was the case before #1086 as well I think. In the former case the param_sets attribute is broken so that needs to be dealt with.

There's really no way to know how many parameter sets a model instance has unless it's specified through the param_dim keyword.

c2 = models.Const1DModel([1, 2], param_dim=2)

This worked as expected initially, then Parametric1DModel set param_dim incorrectly to np.size(parameters_dict[param_names[0]]). The current implementation works because param_dim is set through **constraints which is confusing. I think it should be put back in the constructor. And a test is needed in test_input.

So what kind of output am I supposed to get for c1 = models.Const1DModel([1, 2], param_dim=1)?

With param_dim=2, I get f(x=1) and f(x=2), which works very well for pysynphot.

c1 = models.Const1DModel([1, 2], param_dim=1)

should give an error as it does. Pass param_dim to the model constructor when you want multiple models.
That's why I think param_dim should be made visible to the user.

In the case of Const1DModel, should user even have a choice to modify param_dim if it has to be len(amplitude) or raise an error? I agree that param_dim should be visible, but only for models where users can actually change it safely.

Right about Const1D (and most models) but they have to be written like this (some of them are).
param_dim is a base class attribute which is used as a sanity check in cases when models don't define their param_dim and provides a user with an option to create multiple set model even if the model does not implement param_dim (like in the case of Const1D. But I agree it's good to implement this in all models where it's possible.

There's really no way to know how many parameter sets a model instance has unless it's specified through the param_dim keyword.

Using the current API, yes. But I don't know that passing in each parameter as an array is the best way the pass in the values for multiple param sets. Because what if you want something like m = Const1DModel([1, 2], param_dim=1)? I don't think that should be disallowed so long as param_dim is given explicitly.

The current implementation works because param_dim is set through **constraints which is confusing.

That argument should probably just be called **kwargs in that case--it can include constraints, but a few other arguments as well.

@nden said that m = Const1DModel([1, 2], param_dim=1) is not possible, but @embray says otherwise. I am confused again. What is m = Const1DModel([1, 2], param_dim=1) supposed to calculate physically?

I think it should be possible since there is no restriction in general on the shapes of parameter values. If Nadia is saying it shouldn't be possible then there should be a formal way of declaring the allowed size and/or number of dimensions of parameter values. Const1DModel([1, 2], param_dim=1) is a function of one argument that returns a constant pair of values (whether you call it a vector or some other compound numeric type).

Okay, I get your point. Although I cannot visualize what potential use something like this can be, but that certainly does not mean it should not be done.

m = Const1DModel([1,2], param_dim=1)
m(np.arange(3))
[[1,2], [1,2], [1,2]]

p/s: As you know, the code above currently does not work because it gives ValueError: total size of new array must be unchanged but that is how I imagine it would after you fixed it.

Now I am confused. Both Const1DModel and Const2DModel say they return a float.

There is (was) a formal way of declaring the shape and dimension of a parameter. I still say the current behavior is correct but param_dim must be exposed to the user either by allowing it as a separate parameter in the constructor or by renaming **constraints and documenting it. What am I misunderstanding?

I think 3 of us are confusing each other. Should we just meet offline and talk about this? I mean, our offices are just a few steps away.

@plim Why would we do that?

I think we need to have a discussion on the mailing list on what exactly a parameter to a model can be. This is something I'm still planning to add as an "open question" to APE2 (which I will get to probably after the 0.3 release is done).

My feeling about this is that one should be allowed to define models using either scalar or arbitrary compound values for the parameters (so long as they're compatible with each other in the context of a given function, or at least broadcastable into common shape).

For example, I should be allowed to define something like this (forgetting param_dim for the moment):

amplitudes = np.ndarray(
    [[1.0, 2.0,
      3.0, 4.0]])
means = np.ndarray(
    [[0.0, 0.0,
      0.0., 0.0]])
stddevs = np.ndarray(
    [[0.1, 0.2,
      0.3, 0.4]])

g = models.Gaussian1DModel(amplitudes, means, stddevs)

This effectively gives me a "matrix" of Gaussians that can be evaluated/fitted simultaneously. Perhaps even more simply one could have just set means = 0.0 since as long as we're using Numpy it will have no problem broadcasting that to a (2 x 2) array of 0.0.

Basically this is just a generalization of the param_sets feature that already exists, except that you have a collection of parameter sets that have a shape and dimensionality of their own. This is sort of what I've been getting at, though it clashes with the existing param_sets feature, which is why the example you originally gave is currently broken.

I still say the current behavior is correct

Well, no, if we don't want to allow compound parameters for now if we can't allow the user to do something like m = Const1DModel([1, 2], param_dim=1). We have to go back to the old behavior which is that most individual model classes don't accept param_dim as an argument to their __init__ at all, and instead either fix it to 1 (confusing that some models can have multiple param sets and others can't, but fine until we work out a better solution) or set the param_dim based on the number of elements in the parameter values.

In the latter case something like my above Gaussian example would be allowed, though it would ignore the shape of the parameter values and just go by the number of elements (in other words it doesn't matter whether you set amplitudes = [[1, 2], [3, 4]] or amplitudes = [1, 2, 3, 4]--it will use param_dim=4 in either case and forget the original dimensionality of the input. Also every parameter would have to be a 4 element value. I think that was the original behavior.

So for now I think I will return it to the behavior described in the previous 2 paragraphs since they fit better with the existing design. But I would like to revisit this question...

@embray Speaking of designs, I can't agree more with what you are describing since this is the original design.

@plim great idea!

Right. I think that will work better for now. But I think it's a little confusing in principle.

Two things I do want to try to keep, which I don't think will be hard:

1. The shapes of parameter input values are preserved. If I pass in:

>>> m = Const1DModel([[1, 2], [3, 4]])

the param_dim will be taken as 4 (this is the old behavior that @nden prefers). When returning the param_sets this shape is ignored, but when directly returning the parameter its shape is preserved:

>>> m.param_dim
4
>>> m.param_sets
array([[ 1., 2., 3., 4.]])
>>> m.amplitude.value
array([[ 1., 2.],
           [ 3., 4.]])

1. If a model has param_dim > 1, any parameters that are passed in as scalars have that value automatically replicated across all param sets. For example:

>>> g = models.Gaussian1DModel(amplitude=[1.0, 0.5], mean=0.0, stddev=[0.1, 0.2])
>>> g.param_dim
2
>>> g.amplitude.value
array([1.0, 0.5])
>>> g.mean.value
0.0
>>> g.param_sets
array([[ 1.0, 0.0, 0.1],
           [ 0.5, 0.0, 0.2]])

You can see in this example that mean=0.0 is copied across all param sets.

Slight modification to the second example: For g.mean.value it would still return [0.0, 0.0]. The mean in each param set still needs to be settable independently after the initial assignment. It's just a shortcut...

Okay, I have this mostly working except for one test, and I think this test is responsible for a lot of the confusion about this:

This test claims it should be possible to do something like:

l2 = TestParModel(coeff=[[1,2], [3,4]], e=(2,3), param_dim=2)

Here we have two parameter sets, and the "coeff" parameter is passed in two values: [1, 2], and [3, 4]. Nowhere in the definition of TestParModel does it suggest that the values of its "coeff" parameter should be an array of two numbers. Sure we can guess: "Well, param_dim is 2, and there two values in coeff. So it must be that the size and shape of the values passed in to coeff is correct."

But again, nowhere does it make explicit that each value for "coeff" should be in the form [a, b]. What if we actually intended each value of "coeff" to be a 2d array like [[a, b], [c, d]]. Then the correct input for param_dim=2 should be [[[a, b], [c, d]], [[e, f], [g, h]]]. Both inputs would be accepted since their first dimension matches param_dim, but only the latter would actually be correct in this case.

I guess I just don't understand what use case this test is intended to cover, as it seems to contradict with most other use cases...

This use case, by the way, suggests that it should also be possible to do what originally brought up this issue:

>>> m = TestParModel(coeff=[1, 2], e=1, param_dim=1)

or even

>>> m = Const1DModel(amplitude=[1, 2], param_dim=1)

Update: Pre-#1086 the first case in this comment, using the TestParModel, actually works. Suggesting that my previous notion about how parameters work is fine--they can be allowed, in principle, to be compound types of some sort and they aren't necessarily bound by the param_dim. The second case, with Const1DModel, is disallowed but only by "accident" in that it results in the param_dim argument being duplicated. You get TypeError: __init__() got multiple values for keyword argument 'param_dim'

So basically, for general subclasses of ParametricModel it's conceivably allowable to have non-scalar parameter values. But for all the models that subclass Parametric1DModel or Parametric2DModel their values are artificially limited to scalars. I think this must be intentional, but it seems semi-arbitrary to me. Why make this restriction?

I'm fine with either way. This test case just makes it unclear what the actual intended behavior is...

@embray @pllim @nden - can you clarify where this is? Can it be addressed in time for an RC release early next week, or should we push it to 0.3.1?

I think it is OK to release the code as it is. And push this for discussion for 0.3.1.

@nden - thanks - postponed!

Hold on...I'm still trying to fix this one. If nothing else I want to get it back to the previous functionality, though I don't fully understand the intent behind it.

Okay, as far as I can tell the changes in this PR effectively revert the behavior of param_dim != 1 to how it worked before #1086, with the two exceptions I mentioned: If param_dim > 1, but one of the parameters is given just a single value, it will use that value by default for all param sets; and the shapes of input parameter values are preserved.

👍

Sounds good - feel free to merge once it passes.

Alright, looks like the tests passed, so merging.

😌💦