search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: Fitspy: A Python package for spectral decomposition #5868

Closed editorialbot closed 7 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@patquem<!--end-author-handle-- (Patrick Quéméré) Repository: https://github.com/CEA-MetroCarac/fitspy Branch with paper.md (empty if default branch): Version: 2024.4 Editor: !--editor-->@phibeck<!--end-editor-- Reviewers: @maurov, @FCMeng Archive: 10.5281/zenodo.10812332

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/971a02868d903c0b7c0cbc3d1cd3d139"><img src="https://joss.theoj.org/papers/971a02868d903c0b7c0cbc3d1cd3d139/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/971a02868d903c0b7c0cbc3d1cd3d139/status.svg)](https://joss.theoj.org/papers/971a02868d903c0b7c0cbc3d1cd3d139)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@maurov & @FCMeng, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @phibeck know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @maurov

📝 Checklist for @FCMeng

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.04 s (631.3 files/s, 105279.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          19            667            770           2284
Markdown                         3            133              0            256
JSON                             1              0              0            251
TeX                              1              3              0             80
YAML                             2              5              4             47
TOML                             1              0              0              3
-------------------------------------------------------------------------------
SUM:                            27            808            774           2921
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.5281/zenodo.11813 is OK
- 10.5281/zenodo.8092679 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

Wordcount for paper.md is 1201

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

phibeck commented 1 year ago

@maurov, @FCMeng, thanks again for reviewing! Please go ahead and create your checklist first using the command @editorialbot generate my checklist. Feel free to create issues in project repository directly or write them down as comments here, but please do link the issues in this review so it's easy to follow for everyone. Please let me know if you have any questions about the review process.

maurov commented 1 year ago

@phibeck thanks for the reminder. Sorry, I have been very busy these days. I hope to start the review by next week.

patquem commented 1 year ago

Hello @phibeck, Many thanks for your follow-up and reminders. As a potential reviewer, I forgot to mention @teuben, who had few words about my draft the day I posted it (see https://github.com/openjournals/joss-reviews/issues/5655). He was interested in the Fitspy tool and I just saw he is listed as a reviewer. Feel free to invite him to review the paper.

phibeck commented 1 year ago

Thanks for the suggestion @patquem. Since we're still at an early stage of the review, adding another reviewer probably won't delay the process significantly.

:wave: @teuben, would you be willing to review this submission for JOSS?

phibeck commented 1 year ago

:wave: @maurov & @FCMeng How are things going? Let me know if there are any questions.

phibeck commented 1 year ago

👋 Hi @maurov & @FCMeng it would be great to hear from you when you plan to start your reviews or if there's anything that's holding you up. Thanks!

maurov commented 1 year ago

@phibeck thanks for the reminder. I apologize for taking so long in reviewing this. From my side, there is nothing special holding me up, simply the fact that I am very busy with my official work. I will try doing this task as soon as possible.

maurov commented 1 year ago

Review checklist for @maurov

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

maurov commented 1 year ago

@patquem @phibeck

I have started reviewing fitspy by reading the paper and the (short) documentation. I will install and play with the examples (plus trying some of my own data) soon, but I would like to express my general comments first.

A graphical interface on top of lmfit may be of interest for those users who want to build a quick fitting model and apply it on multiple similar spectra, without prior knowledge of Python programming. I support this idea, but this software should be put in the right context and clearly stated in the paper/documentation. In fact, in addition to what here is called "spectral decomposition", "peak fitting" or "curve fitting" is performed in any field of science. This means that there already many (open source) softwares available and almost every researcher/community has a preferred tool. Clarifying the specific problems fitspy solves (a GUI for Lmfit?) and to which research community is targeted (X-ray fluorescence?) is mandatory. For example, why not comparing fitspy design to Fityk? Why two fancy astrophysics tools (specutils and pyspeckit) are cited (apart the obvious fact that they do peak fitting)?

In conclusion, the current "Statement of need" is too generic and unclear. I am asking myself if there is "substantial scholarly effort" behind this project. Is this a "yet another peak fitting tool"?

FCMeng commented 1 year ago

@phibeck Apologize for the late response. I have been really stacked this semester. I will start the review process as soon as possible.

FCMeng commented 1 year ago

Review checklist for @FCMeng

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

patquem commented 12 months ago

Hi, Thank you @maurov for your reviewing and your comments. You mentioned a lack of clarity regarding the objectives of the software and I will try to clarify them. Which communauty is this tool aimed at ? I would tell anyone who needs to do peak fitting (and only peak fitting) in the easiest and the quickest (in term of accessibility) way possible. While I agree with you (there are already many other softwares that do peak fitting and Fitspy may appear like "yet another peak fitting tool"), most of the softwares with a GUI don't manage 2D-maps and/or do not have multiprocessing capabilities (see the (non exhaustive) list of open-source softwares below). Although Fitik that you mentioned is a tool with an advanced GUI and is undeniably more evoluated that Fitspy, it does not have 2D-map and parallel capabilities. The only ones that I have identified and that combine these 2 features are XISMUS and HyperspyUI. From what I have seen, XISMUS is strongly attached to X-Ray Fluorescence datasets. On this point, Fitspy aims to be a generic and agnostic tool. HyperspyUI seems to be the closest tool but (in my experience) is not as easy to use as Fitspy and this tool aims at the end to adress all Hyperspy options, far from the only need of peak fitting to cover. After testing Fitspy, @maurov do you agree to consider Fitspy as satisfying some of the terms used in the "scholarly effort" section : faster and/or easier and/or simpler than most open source software already existing? If so, I will rewrite the "Statement of need", with emphasis on consideration about the dedicated easy-to-use peak fitting tool and its 2D-map/parallel capabilities. image

maurov commented 11 months ago

@patquem I apologize for not being very reactive on this review, but I am very busy these days. I hope to move forward in the next two weeks.

As said in my previous comment, you should clearly give in the "Statement of need" which scientific community this code is targeting and give a real application example. I understand that you want to provide a generic and efficient peak fitting tool, but it is impossible to get all features for all users communities, so the point here is not to demonstrate which software is the best, but just to provide a given tool to a given community. For example, when you refer to 2D maps, are you referring to X-ray fluorescence maps? For such application, one very good open source software that is not in your table is PyMca.

phibeck commented 10 months ago

Thanks @maurov for getting the review started! Have your issues in https://github.com/CEA-MetroCarac/fitspy/issues/9 been addressed? It sounds like there are some clear steps for @patquem to improve the manuscript, too.

@FCMeng could you provide an update on the progress of your review? Thank you!

patquem commented 10 months ago

Hello @maurov, Thank you again for your comments. As previously said, Fitspy (like Fityk) is a generic and agnostic tool dedicated to do peak fitting and only peak fitting and is not intended for any specific community or specific applications. To justify my point and answer to your questions, I would say I work in a multidisciplinary institute working on many fields like biology, batteries and semi-conductor with a wide range of characterization techniques. Over the last 5-6 months, Fitspy has been used to process spectra issued from Raman, PL, XPS, XRD and EELS to characterize materials from the range of several microns to the range of thousands of mm (wafers). The number of spectra processed by Fitspy during these studies varied from a few to several hundred thousand. The 2D map characterisation was used both for the characterisation of PL spectra (at the wafer scale) and to process EELS spectra at the micrometer scale. Fitspy has been used by both experienced and interns who have appreciated the tool for its “easy-to-use” compared to the reference tools they currently use (as they said: “reference tools offer a large range of services but, as a consequence, are more complex and not so ergonomic to use for such basic and (often) repetitive tasks that peak fitting operations are”). These are the reasons why it does not seem appropriate to me to focus the Fitspy “statement of need” on a particular community and specific applications.

patquem commented 10 months ago

@maurov @FCMeng and @phibeck : With the revisited Statement of need below: Does the article have a chance of being published or do you consider it as definitively rejected ? Indeed, I need an answer because articles issued from applicative studies with Fitspy are now about to be submitted and authors need to know if a Fistpy reference does exist (Initially, in July, when I submitted the present article to JOSS, I was thinking IT could be the reference paper). Thank you.

patquem commented 10 months ago

Statement of need (revisited)

Numerous open-source tools for spectral fitting exist. However, most of them have been designed for specific application domains, offering a broad range of services beyond mere spectral fitting. Consequently, these tools can prove challenging to use, especially for less experienced individuals.

In the vein of generic tools like Fityk or PRISMA, Fitspy is a dedicated tool for spectral fitting—and only spectral fitting—with the following characteristics or functionalities:

Agnostic Nature: Fitspy is not tied to any specific physical quantity or database. It is designed to process spectra regardless of their x-support and y-intensity without requiring prior knowledge.

Python Implementation: Fitspy is coded in Python. As a result, spectra can be easily processed using Python scripts, catering to individuals with basic knowledge of the language.

2D Maps: Fitspy has been designed to handle spectra derived from 2D acquisitions. Note that beyond "2D", dimensions can encompass time or any other dimension. When dealing with 2D data, an interactive map in the Fitspy GUI allows users to locate and select spectra of interest easily.

Multiprocessing Capabilities: Fitspy enables spectral fit processing on multiple processors, enhancing efficiency.

Constrained Parameters: Leveraging the Lmfit library, Fitspy empowers users to impose constraints on parameter ranges or establish constraints between parameters using literal expressions.

Simple GUI: Fitspy has been designed to be as intuitive and simple to use as possible (subjective criterion).

To the author's knowledge, although many open-source software applications are more advanced in certain aspects mentioned, none of them appears to encompass all the functionalities described above.

maurov commented 10 months ago

@patquem

thanks for the revised statement of need and I also appreciate the paragraph showing a use case for Fitspy. This is what I was proposing since the beginning: say how your current users use this software for. Please, insert them directly into the revised manuscript and regenerate it.

@patquem @phibeck

I acknowledge the effort for providing this peak-fitting tool to the community and I think this work fits the scope and audience of JOSS. I may be in favor of accepting this contribution, but only once the documentation is improved and extended. I have opened a specif issue for it with some ideas, even though my time is very limited and the list is far from being complete and exhaustive. Overall, the provided "README" file mainly describing what the buttons on the GUI do is not enough.

@phibeck

I went trough all the points, but some of them are still not checked because are currently not met, in my opinion.

phibeck commented 10 months ago

Thanks @patquem and @maurov for the updates. Regarding the scholarly effort (and after consulting with other editors), it is not so much a question of whether there are alternative packages, but whether the amount of work done by the authors meets the minimal requirements. The editors already reviewed the scope in a first round and found that it meets the requirements, but if reviewers have strong doubts these should be stated.

Given the assessment by @maurov it sounds like the article can be published once the issues are taken care of by @patquem. However, we need at least a second report, and I hope @FCMeng is still available to give one.

FCMeng commented 10 months ago

@phibeck I am really sorry for the delay. The semester was really busy. I should still be able to provide my report during the holidays.

phibeck commented 10 months ago

:wave: Hi @FCMeng could we get an update when you plan to start your review? Thanks!

:wave: @patquem could you let us know where you stand with responding to the latest issue created by @maurov? Thank you!

patquem commented 10 months ago

Hi @phibeck, I am working on the article and a static HTML document (as previously requested by @maurov), which is taking a little bit of time. I plan to submit the final version of the article with my reply to the @maurov 's requests by the end of the week.

patquem commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

patquem commented 10 months ago

Hello @phibeck, @maurov and @FCMeng, As agreed, regarding the article, the 'statement of needs' section has been revised, and an illustration corresponding to a use case has been added. The documentation is now in the form of a static HTML Sphinx doc. Fitspy has also undergone some improvements, including the ability for users to input their own models. A new version of fitspy (v2024.1) has just been deposited on PyPI today. Regarding the issues raised by @maurov, see my replies here. Hoping that the project's status and the article meet your expectations to authorize the upcoming publication in JOSS.

phibeck commented 10 months ago

Thank you for the update @patquem. @FCMeng and @maurov when you have a moment please review the recent changes and let us know where you stand in the review process, thank you.

phibeck commented 9 months ago

:wave: @FCMeng and @maurov just checking in to see if you've had a chance to continue your review. Thanks!

patquem commented 9 months ago

Thanks @phibeck for your supervision. From my side, I continue to improve Fitspy with a version v2024.3 that should be released soon. In the meantime, here's a new version of the draft that emphasizes (in ~one additional sentence) the ability to input 'user' models (available from v2024.2).

patquem commented 9 months ago

@editorialbot generate pdf

editorialbot commented 9 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

maurov commented 9 months ago

@phibeck thanks for the reminder and I apologize for the long silence (busy period!). I have checked the latest improvements made by @patquem and I appreciate the efforts done. The documentation is more complete now and the paper was improved. Green light from my side to accept this contribution.

phibeck commented 9 months ago

@maurov great, thank you very much for your review! I see that the Performance bullet point is still unchecked on your checklist. May I ask you to let me know whether the performance check still needs to be completed, and if not, could you please mark it as such? Thank you!

Thank you @patquem for the update as well. I do hope that @FCMeng will be able to work on their review within the next two weeks in order to move forward with this submission.

maurov commented 9 months ago

@phibeck I did not have the time to check the performance yet. Nevertheless, I believe it fits the author's claim. @patquem if you could add a test file for checking performance, it would be helpful.

phibeck commented 8 months ago

@FCMeng how is your review going? Please let me know if you need more time.

FCMeng commented 8 months ago

@phibeck @patquem Thanks for the reminder. My review process is almost done. I agree this is an excellent work and fits the scope for JOSS. I do have a few questions when testing with my own data.

signal.find_peaks() is used to find the peaks in the spectra. If the resolution is not good enough, some peaks might be ignored in this process, especially small "shoulder" peak near the main peak. In this case, is it possible to do an auto interpolation first and then fit on the interpolated spectra? Or is it possible to give the users option to interpolate the spectra in fitspy?

I noticed there is small portion of todo list on the developer part in the main documentation. A coding/debugging guideline for developers/contributors is strongly recommended, which benefit the community.

The supported operating systems and python version should be provided. When I was trying to use fitspy on Macbook with M2 chip, the installation process succeeded but have issue when launching fitspy.

Is the size of spectra figure in the GUI fixed? I am facing an issue use the GUI on a Linux machine (Rocky OS), where the size of the spectra figure will not be reshaped automatically when I resize the GUI window. In this case, I can not see the whole spectra or some part of the menu on the right side of the spectra figure if the GUI window is too small.

In the instructions for the test, it mentioned run python example/ex_gui_auto_decomposition.py, where it should be examples.

phibeck commented 8 months ago

Thanks @FCMeng for these comments. @patquem please keep us updated when working through these issues.

Also checking in with @maurov regarding the performance check before signing off on the review.

patquem commented 8 months ago

Hello everyone,

To begin, I am pleased to announce the release today of the v2024.4 which includes a number of new features (like outlier detection, noise level evaluation, ...) and performance improvements since the beginning of the year.

Regarding your last feedbacks:

@maurov

Performance: The performance are those of lmfit (Fitspy does not incur any additional costs at the fitting level). However, I am open to suggestions on what could constitute performance tests (I have not found such test in PyMca (?)).

@FCMeng

Interpolation: I am not sure to have understood. how this would help in extracting the small shoulders. (Feel free to provide explanations in the Fitspy Issues section, which could potentially become a new feature later on).

Coding/debugging guideline: I have added a few lines on this topic. Please note, however, that Fitspy is not intended to become larger, as the very principle of the code is to remain as generic and simple to use as possible :).

Operating system: I apologize for the issues you encountered on your MacBook. I will try to test the tool on other Operating Systems than mine, but Macs are not very common around me.

Fixed size: Yes (unfortunately), this is a problem that has already been raised by some users a few months ago. I plan to switch from Tkinter to PySide in the coming months to address (among other things) these display issues. But it won't be happening right away.

Confusion about tests and examples: If you are referring to what is mentioned in the README, the section title refers to "Tests and examples execution," which includes both "pytest" and an example of running one of the Fitspy examples.

I hope these responses will meet your satisfaction, and that the article can be published in the coming days. Indeed, there are articles from Fitspy users that are currently under review, and it would be nice to include the reference to the present JOSS article in their publications.

Anyway, Thank you very much @FCMeng and @maurov for the time you have dedicated to this review. Your feedback has greatly contributed to the improvement of Fitspy. And a big thank to @phibeck for your relentless support during these last 7 months.

Patrick

phibeck commented 8 months ago

Thank you for the extensive update @patquem! @FCMeng please let us know whether this addresses your initial comments and if there are any other issues working through your checklist.

@maurov please also update us whether you are satisfied with the performance assessment. Thanks!

phibeck commented 8 months ago

Just clarifying, @fanchen0121 are you the same user as @FCMeng?

Edit: can be ignored, see next message.

FCMeng commented 8 months ago

@patquem Thanks for your detailed responses.

  1. Let me rephrase the first question in this way: can signal.find_peaks() always return the correct peaks? If not, any ways to improve it?
  2. The last minor question I was referring here: https://github.com/CEA-MetroCarac/fitspy?tab=readme-ov-file#tests-and-examples-execution where the last command python example/ex_gui_auto_decomposition.py should be python examples/ex_gui_auto_decomposition.py.

@phibeck My bad! I forgot to switch account. I have deleted the previous comment and repost it here with the correct account.

patquem commented 8 months ago

@FCMeng

1. can signal.find_peaks() always return the correct peaks? If not, any ways to improve it? find_peaks() return peaks according to its possibilities (finding local maxima by simple comparison of neighboring values, given the different parameters that can be adjusted by the user). This method may work well for distinct and separated peaks. However, for merged peaks, the task becomes significantly more complex and may require users' expertise. (Practically, people using fitspy around me, set the peaks manually).

2. exampleS in the README You are right. This has been fixed.

@maurov

Performance A brief section with some info about performance has been added in the documentation here

maurov commented 8 months ago

@phibeck @patquem sorry for the late reply. This conversation got tagged as "Spam" and I did not receive last messages until today.

@patquem thanks for your comments about performance, it is fine for me.

phibeck commented 8 months ago

Thank you, @maurov for completing your checklist and for the thorough review! :tada: @FCMeng keep us updated whenever you have a moment to review the remaining points on your checklist, thanks!

FCMeng commented 8 months ago

@phibeck I am good with the responses and have updated my checklist.

phibeck commented 8 months ago

Thank you @FCMeng for the update and for completing your checklist, and many thanks to all reviewers (@maurov) for your help with this submission! 🎉

@patquem the reviewers have recommended the submission for publication. There are a few more steps before we finalize the publication.