search

openmrslab / suspect

MRS processing tools
https://suspect.readthedocs.io/en/latest/
MIT License
33 stars 23 forks source link

Allow update on lmfit's version #162

Closed darrencl closed 6 months ago

darrencl commented 1 year ago

Hi @bennyrowland

In this commit, it seems that the lmfit is pinned at version 1.0.0 with the reason it breaks ndarray subclasses. May I ask what is the detail of the breaking situation and is there any fix so we can use the newer version of lmfit?

In our project, we used lmfit.minimize with least squares and it seems there is a bit of difference (although it's very small and not noticeable) in result on lmfit version 1.0.0 and SciPy 1.5.0rc1 vs when updated to lmfit 1.1.0 and SciPy 1.6.0. I think it could be related to one of either or both packages updates. Looking into SciPy's 1.6.0 release note in this part, it seems to have update the least square. Also, in this lmfit's commit, it seems to add the keyword when it uses least square method.

So, I think with the updates (potential fix or improvement) on these packages, I think if possible it would be great to not pin the lmfit's version to 1.0.0, which was released in 2019.

bennyrowland commented 1 year ago

@darrencl sorry for the slow response. This is a pretty old issue and I had to scratch my head to try and figure out what the problem was (and promise to write better commit messages in the future). In the end I found this issue that I filed at the time with lmfit: https://github.com/lmfit/lmfit-py/issues/767. Basically the problem is the coercion of the ndarray to a specific datatype using np.asfarray() which removes the subclass. The subclass is necessary because calculating a peak requires knowing the bandwidth of the FID. I got a "won't fix" response from them and haven't worried about it because I am no longer using suspect myself. It should be possible to modify the code to pass in the bandwidth (or equivalent parameter) into the function as a separate constant or something like that, but it would require fiddling about somewhat.

Personally, I consider the best approach would be a complete re-write of suspect to use xarray.DataArray as its base data container and xarray's data accessor methods to provide MRS specific functionality. This would have the inconvenience of an extra qualifier in various places: data.mrs.time_axis() rather than data.time_axis(), for example, but would otherwise give the advantages of encapsulation over subclassing. However, I don't have the bandwidth or the motivation to undertake this by myself. If you are interested in continuing to use suspect over the longer term then I am happy to help you to improve it though.