-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: FASMA 2.0: A Python package for stellar parameters and chemical abundances #2048
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @warrickball, @ipelupessy it looks like you're currently assigned to review this paper 🎉. ⭐ Important ⭐ If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿 To fix this do the following two things:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
@MariaTsantaki, @warrickball, @ipelupessy : this is the review thread for the paper. All of our communications will happen here from now on. Both reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines. The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time. Please feel free to ping me (@xuanxu) if you have any questions/concerns. |
@MariaTsantaki I think the install procedure should not require sudo |
@MariaTsantaki I see that the install procedure calls for editing the source code of MOOG by hand and then build/installing this seperately; it would be nicer if the setup.py does this for you (both things). Also the model data extraction can also be put in the setup.py. fixing this would allow a |
@MariaTsantaki at first sight I see only two tests in FASMA/tests. is this correct? |
@MariaTsantaki the repository contains compiled .o and executable files of MOOG. I would suggest these be removed. |
@ipelupessy (I'm co-author of FASMA). It is true that there currently only are two unit tests. We plan to add more continously, but it hasn't been a priority. |
@ipelupessy Thanks for reviewing! It's true, only 2 tests. My reason is that errors can be checked by modifying the configuration file and running fasma. But I can include more tests if it is more efficient for the testing. |
ok, you need at least basic tests where you go through some simple cases (confirming the install went ok etc) - this will help you enormously when you change something in the install, making sure nothing breaks.. another comment: have you tried to minimize your prerequisites? e.g. I think at the moment astropy is only needed for for fits support - its a big prereq. for a small task..your package is not too bad..but in general I am in favour of minimizing dpendencies;-) I will try compile and run something later so I can comment more substantially... |
@MariaTsantaki I've just tried to install the program following the instructions on the GitHub repo. Like @ipelupessy, it shouldn't be necessary to install with First, this doesn't make use of any of Make's dependency handling and doesn't do anything more than a bash script, so if the script remains necessary I think you should just run it as a bash script. But the contents are so simple that it would be better off as simple installation instructions. Secondly, the
With the I'm also having trouble installing MOOG but, since that's actually stopping me from progressing, I've opened an issue in the repo itself. |
I'm not familiar with everything that |
@MariaTsantaki as @warrickball remarked, the installation instructions are not complete. You need sm plotting package. I am not sure the plotting is needed; in any case it compiles if you add a .f file with stubs for the sm calls. Then, when running fasma, MOOG fails because missing data (Barklem.dat), I pulled this from https://github.com/hmtabernero/StePar (there is also a stub library for sm there; I don't think this lib works as a general solution because it is written in c and assumes a particular fortran name mangling), after this it seems to work. |
some comments on running:
fasma then complaints about a missing intervals file. so i copied over intervals.lst. it still complained. Error reporting doesn't actually tell me which file was missing, added this in the error message (see PR MariaTsantaki/FASMA-synthesis#13 ). turns out the default suggested by the documentation is not correct (intervals_hr10_15n.lst)
|
@MariaTsantaki some further comments: |
MariaTsantaki/FASMA-synthesis#15 contains the above mentioned stub for sm |
Hi all, |
Dear reviewers,
There were indeed some dependencies which were not used and now are removed. Unfortunately, I could not remove for instance astropy as it manipulates fits files and I am not familiar with other methods dealing with them. A good test (once installation runs fine) will be to adapt the config.yml with the correct paths (for linelist, intervals_file, observations) and change the option 'minimize' to True. Then by running fasma the parameters of the Sun will be obtained! Thank you very much for your time! |
The test run of the Sun runs fine but I tried to run using a HARPS spectrum of HD38529 that I downloaded off the ESO Archive and got the error
I presume that FASMA expects the spectrum to be in a particular format (that includes the Here are the contents of
|
I managed to install and run with the provided instructions. The path limit is annoying; I have made a PR (MariaTsantaki/FASMA-synthesis#17) to fix this..it adds another change in the MOOG source though (maybe its good to communicate it upstream to MOOG developer) I can confirm @warrickball problem; I think the fits file does have a WAVELMIN keyword in the header with the smae info as CRVAL1 ?? |
one comment about the code paper proof: it doesn't mention what the improvement/changes are from the previous published version. I think this is needed. |
The following issues are open from my checklist: Functionality.Functionality: related to documentation below I am not sure what all the functionality of the packages is; Documentation.A statement of need: Its not clearly stated what the problem the software is designed to solve, e.g.: "FASMA delivers the atmospheric stellar parameters (effective temperature, surface gravity, metallicity, microturbulence, macroturbulence, rotational velocity) and chemical abundances of 12 elements." -> from what? Reader has to guess. What is the fundamental method of the package (multidimensional fit?) Documentation.Functionality documentation: The manual provides a description of the configuration file. missing is a high level description of what the config.yml is needed for (e.g.: "The configuration file provides the input for the fitting procedure and..?") Documentation.Automated tests: please provide if there are any tests beyond the basic example. (if not its ok) Documentation.Community guidelines: not clear how to contribute Software paper.Summary: atm is geared to specialist. Please provide one or two lines describing the software for non-specialist Software paper.A statement of need: both the problem and the audience can be clarified a bit more These are not huge issues, but I think they should be adressed to make the functionality of the software a bit more clear. This concludes my review. |
OK. 10.5281/zenodo.3885291 is the archive. |
@whedon accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#1469 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1469, then you can now move forward with accepting the submission by compiling again with the flag
|
Hi @MariaTsantaki, we are about ready to wrap this up, but in my review of the article I encountered some minor issues. Could you address these and let us know when you have done so?
Thanks! |
Done |
@whedon generate pdf |
@whedon accept |
|
|
@whedon accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#1476 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1476, then you can now move forward with accepting the submission by compiling again with the flag
|
@whedon accept deposit=true |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? notify your editorial technical team... |
Congrats @MariaTsantaki on your article's publication in JOSS! Many thanks to @warrickball and @ipelupessy for reviewing this, and @xuanxu for editing. |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
Submitting author: @MariaTsantaki (Maria Tsantaki)
Repository: https://github.com/MariaTsantaki/FASMA-synthesis
Version: v2.0
Editor: @xuanxu
Reviewer: @warrickball, @ipelupessy
Archive: 10.5281/zenodo.3885291
Status
Status badge code:
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
@warrickball & @ipelupessy, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @xuanxu know.
✨ Please try and complete your review in the next two weeks ✨
Review checklist for @warrickball
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @ipelupessy
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: