Move mpfit to its own subpackage.

[wtbarnes]

Currently, mpfit is part of the core subpackage. Given the discussion in #10 as well as the fact that this dependency is being vendored (i.e. it is a third-party piece of software being absorbed into the package), I think every effort should be made to disentangle it from the core functionality of the package.

I would recommend either creating an mpfit subpackage or, even better, creating a more generic fitting subpackage with the thinking being that the fitting "engine" could be swapped out for something else later down the line.

Additionally, because this software is being included in eispac, eispac must also include the LICENSE file for astrolibpy, per the terms of the GPL v3 license: https://github.com/segasai/astrolibpy/blob/master/LICENSE.

[kdere]

there is also a package called lmfit https://cars9.uchicago.edu/software/python/lmfit/installation.html Downloading and Installation — Non-Linear Least-Squares Minimization and Curve-Fitting for Python - CARS<https://cars9.uchicago.edu/software/python/lmfit/installation.html> Testing¶. A battery of tests scripts that can be run with the pytest testing framework is distributed with lmfit in the tests folder. These are automatically run as part of the development process. For any release or any master branch from the git repository, running pytest should run all of these tests to completion without errors or failures.. Many of the examples in this documentation are ... cars9.uchicago.edu

…

________________________________ From: Will Barnes ***@***.***> Sent: Sunday, September 5, 2021 1:47 PM To: USNavalResearchLaboratory/eispac ***@***.***> Cc: Subscribed ***@***.***> Subject: [USNavalResearchLaboratory/eispac] Move `mpfit` to its own subpackage. (#16) Currently, mpfit is part of the core subpackage. Given the discussion in #10<https://secure-web.cisco.com/1bb58MxnSkSikxoWFulMtc48_qslt28xJO8jR0t-XuOq58L_B7Z2Yb00lQT3hqDiPNA25bT_zIRoDww3CUZgHd4p0j8WPR6v3wsacqt3S39XCMADNYCEOdIi5pfKO4_viYjYZWRASDjEt_pxBuDXOn9kVLDsfrDC9VEF2vXX3-yVIH1wWVtDr8k44HRef61n58qHsUdlqUwsZVAqFRZ5ss35lsdCmMNhkzvDfBdLrTsf3IhH1BdOSkmWY_StUvOsrbOXpY9bhdTNpgQTSvR_nfeqqt-fcWCJLNL1jVhrlEUhjdjeI8ezA_3iyypQ2pJwLz3pthkBLPQgEhtdj26-q71CNL1ttrLvoAcqiGOTIWgafhj95cBTGKh85KrDNUaab3B5wCW-vSduCjBLNBQgJJ_1UHMIgpBl4-prWqWzcyMHLhyD_L1Dzn2MofsJX0Ar8/https%3A%2F%2Fgithub.com%2FUSNavalResearchLaboratory%2Feispac%2Fissues%2F10> as wel! l as the fact that this dependency is being vendored (i.e. it is a third-party piece of software being absorbed into the package), I think every effort should be made to disentangle it from the core functionality of the package. I would recommend either creating an mpfit subpackage or, even better, creating a more generic fitting subpackage with the thinking being that the fitting "engine" could be swapped out for something else later down the line. Additionally, because this software is being included in eispac, eispac must also include the LICENSE file for astrolibpy, per the terms of the GPL v3 license: https://github.com/segasai/astrolibpy/blob/master/LICENSE<https://secure-web.cisco.com/1MmQYMuF3xijD_BhMgLY8Li4EEQZ5yMDt5qlGIs4fk5kUakyTaaAqWzQCEiSVIjB0cczDLh1XOV8Dum88UoE6ohQDHCrYy7EovjhDkzRqany9yLt0dLSgBrgk2LuAG_73kEFo-cjZUPXcWAmzleKO-BHG3YSUpe3dRQPZrgIxjeSqWEGiFVHC6vGK1TywhdiEu-lVcAI1YNlWUIEZJgVfKCRH25v2jtlYh3GtRfafWTjQQT0gJNffQ0t3EenwwqamQSw4yExP9W8ApQcMDs_0MvNoEsDqUrgFXtPZdI3EQogg0DuItc_IlcHGTNrpEA2hIiGJ-S2nhES_wAkXDwV7_4_lhIhlCiuzpLpLJBQ7fq8bqx8d92VjMsdABR-xYHcQ_4SsKuHbsUw0ZSFwd85Qr2_crWtWAGB8JoZkWeBSjsJXSpyO_s-eT7Ex7bsZGzI-/https%3A%2F%2Fgithub.com%2Fsegasai%2Fastrolibpy%2Fblob%2Fmaster%2FLICENSE>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<https://secure-web.cisco.com/12-tskuMAdzIpo0qeGiGksT81elOdwxdWCKhhKvpzB8xPqM9OPpSOZAjEkrvQY5tHnwP5mpDvrnqgTmDwXRFJo4_8wRJcFXOIguMe9dBqaLLCg2kWzWOhS37r0rIpW1fw05supLjqsTOWztpAuRFYFud4JOSQOjcdsHzNVJWBjzsgBJ3nZPdeNSptKTxNes7pMlE_-Hv5qwi9Iqen5tUf0HuJtxlgAQ6xVK1YxBQYFi-z9r05RVSUblRMK24KBZcU0XuTgBvHucGwrrDIZpaBFGfXudSKseZhEj5LM075Ya-MDkm22mURowV8bSJ7QRa_4l1mUFCXc79AijchLCrQiOIFGIumCEJQpYTUqhzZDSS5TrikTWRtyNV5d_tp2zlGj-CJvtjE8oaFvHZx0mud23heBRqA8dAq7GWiPxdV8bXXy4kqPUzf0ZxSYU5Eq71s/https%3A%2F%2Fgithub.com%2FUSNavalResearchLaboratory%2Feispac%2Fissues%2F16>, or unsubscribe<https://secure-web.cisco.com/1_Mphl__V4xHhVWCuFnPeA8EQEOjr3U-ISk1KcloV9xFCyBD753o1wSdxamIKT3wUX8nrh5YSpIgavlLeDR0YT35bLyknWXjGp7hyxvY_7I_knw3AP1_bMslTQQ6jSssoJmoCh6LYv3ghREVOd_Pq3GBSJqgX9XIk1GHJCD9tgn8qLiZe! %20AJOOEQm9NmGDhqOoPH04V3ZmMi_1cAWL0QD2ZRLSfdKMn4-YLssSscxFncICH2GBpJxojzeEd4P8BwaBsI8LJZ7TQ2g8nHhuehC0RXzmLL43fuPCtXsAsu9e-JS5bh_qZSzCTnO8YTE39WYoQGmv-Jev4zyPId42R3Go0BUx78FWu-T91H_0zMZfYmQZ7eSqD-hYeh9Uzoqxrgt3InX1Nfujr59ddp8RFdKCwMLGWfSgdLq3Ep0vT55rIHg_LC6W3oq6mDOUBKdav9c6/https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAFUQVVXKABBOIZQ2AT6QETUAOUKTANCNFSM5DPA46NQ>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[PaulJWright]

Hi @MJWeberg. I agree with @wtbarnes here, and would ideally like this addressed as part of the open review: openjournals/joss-reviews#4914, and please include the LICENSE file for astrolibpy

[MJWeberg]

Hello @PaulJWright. We debated moving MPFIT to its own subpackage a while back, but decided to wait until we could also add support for other fitting backends. Fitting is a core component of EISPAC, so the fit_spectra() function will most likely remain in the top-level namespace. Since users don't actually ever call MPFIT directly, moving it to a separate directory would primarily be an internal restructure anyways. Nevertheless, we are happy to go ahead and move it now, if you think it is best.

It was not clear what open license mpfit.py actually fell under, since the GPL v3 license of astrolibpy is only for the other components of the package. I thought it was public domain, but apparently it is slight more complicated than that. I have added an MPFIT-LICENSE file that contains the required information to the best of my knowledge.

Unfortunately, we have not had the opportunity to extend the fitting backend options yet. Since some of this conversation took place off-line, I am including a summary below for more complete records.

When we first wrote EISPAC, none of the other fitting packages were quite sufficient for our needs. In particular,

• scipy.optimize.leastsq does not allow for tying parameters (as discussed in mpfit.py #10)
• lmfit does not provided a generalized Gaussian function (e.g. it scales all the amplitudes). This is inconsistent with all previous EIS analysis software and would likely cause confusion.
• Astropy.modeling was close but (a) was very slow, (b) not easily parallelized, (c) would sometimes inexplicably fail to converge.

I understand there may have been some improvements to the Astropy subpackages since then, so I will try to take another look at it sometime in the next month or two.

[PaulJWright]

As @wtbarnes opened this particular issue, I'll let Will have the final say, but it seems quite odd that a third-party piece of software is incorporated into the core subpackage (@nabobalis is more experienced here, and may have a different opinion). Personally, I would restructure (not asking you to create a bunch of additional fitting functionality), but happy to hear @wtbarnes and @nabobalis opinions on this!

[nabobalis]

I am not sure what the license and/or legal implications of adding mpfit directly to the core folder as it doesn't seem to be actually licensed.

When it comes to adding external code, "normally" it is kept in a separate folder that makes it clear that this code was not written by the original authors.
This is done in sunpy and astropy for example which bundle both extra Python libraries and C code.

How wide spread this practice is, I am unsure.

[MJWeberg]

Moving mpfit to a more general extern subpackage makes a lot of sense! It removes the confusion, allows for the inclusion of other third-party code (although I don't foresee us adding anything else at the moment), and gives us more flexibility for how to handle additional fitting backends somewhere down the line. Thanks for the suggestion, @nabobalis (and sorry @wtbarnes, for not understanding this was part of what you were originally suggesting).