QENSlibrary / QENSmodels

Models which can be used to fit QENS data
https://qensmodels.readthedocs.io
BSD 3-Clause "New" or "Revised" License
0 stars 3 forks source link

Add area normalization to Gaussian and Lorentzian when area > 1 #26

Closed gonzalezma closed 2 years ago

gonzalezma commented 2 years ago

To address issue 24

gonzalezma commented 2 years ago

I think this should work. What do you think?

gonzalezma commented 2 years ago

It still remains the possible issue that doing x = 0.0 lor = QENSmodels.lorentzian(x, 1.0, 0.0, 0.2) print(lor.shape, type(lor)) --> () <class 'numpy.float64'> while x = [0.0] lor = QENSmodels.lorentzian(x, 1.0, 0.0, 0.2) print(lor.shape, type(lor)) --> (1,) <class 'numpy.ndarray'>

As the returned object in the 1st case is not a numpy array, I wonder if in some circumstances similar errors could not appear.

gonzalezma commented 2 years ago

Do you think that the input validation should be changed to: x = np.asarray([x]) This seems to force x to by an array of shape (1,) even when x is a single float.

celinedurniak commented 2 years ago

Can't we change the test to check x.shape != () instead of using its size?

gonzalezma commented 2 years ago

Yes, we can. But I just tried using x = np.asarray([x]) and it seems that with this we can ensure that we have a np array (of shape (1,) if x is a float) and we can then remove the check completely. Or do you see any new problem appearing because of this?

celinedurniak commented 2 years ago

I tried your solution. It is fine if x is a number. But if it is x = [0, 1, 2, 3, 4, 5], lorentzian(x, 0.3, 0.4, 0.0) returns array([0., 0., 0., 0., 0., 0.]) instead of array([0.3, 0. , 0. , 0. , 0. , 0. ])

gonzalezma commented 2 years ago

Yes, of course. This changes the shape from (6,) to (1,6) so it's not good. From your perspective, what is better:

  1. Going on with the solution of the test before trying to calculate the area (BTW, why do you prefer the test x.shape != ( ) over x.size > 1)?
  2. Extend the input validation to ensure that x is always a 1D numpy array of shape (1,) for a single float or integer input and (N,) if the input is a list of values or a numpy array. I guess we should also check that all are valid numbers, e.g. not NaN or inf in the input?
celinedurniak commented 2 years ago

I think for the scope of this PR, 1) is enough. But we should create an issue in order to implement 2) for all models (i.e. creating a validate_input function which is applied to each model). I do not have any preference for the test between .shape and .size. We can leave it as it is now.

gonzalezma commented 2 years ago

That's a good plan. For the test, I don't have any preference either. I just wrote the first thing that came to my mind, but I trust you more to write good and reliable code, so if you feel that it's better to change it, that's perfectly fine.

celinedurniak commented 2 years ago

It is fine for me as it is.