[REVIEW]: FASMA 2.0: A Python package for stellar parameters and chemical abundances

[whedon]

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:

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

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:

1. Make sure you're logged in to your GitHub account
2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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

• [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• [x] Contribution and authorship: Has the submitting author (@MariaTsantaki) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
• [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

• [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
• [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
• [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @ipelupessy

Conflict of interest

• [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• [x] Contribution and authorship: Has the submitting author (@MariaTsantaki) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
• [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

• [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
• [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
• [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

[whedon]

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 :tada:.

:star: Important :star:

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:

1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

2. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

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

@whedon commands

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

@whedon generate pdf

[whedon]

Reference check summary:

OK DOIs

- 10.1086/152374 is OK
- 10.1051/0004-6361:200809724 is OK
- 10.1093/mnras/stx2564 is OK
- 10.1051/0004-6361/201527120 is OK
- 10.1137/0111030 is OK
- 10.1088/0031-8949/90/5/054005 is OK

MISSING DOIs

- None

INVALID DOIs

- None

[whedon]

:point_right: Check article proof :page_facing_up: :point_left:

[xuanxu]

@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 openjournals/joss-reviews#2048 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

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.

[ipelupessy]

@MariaTsantaki I think the install procedure should not require sudo

[ipelupessy]

@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 pip install FASMA

[ipelupessy]

@MariaTsantaki at first sight I see only two tests in FASMA/tests. is this correct?

[ipelupessy]

@MariaTsantaki the repository contains compiled .o and executable files of MOOG. I would suggest these be removed.

[DanielAndreasen]

@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.

[MariaTsantaki]

@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.

[ipelupessy]

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...

[warrickball]

@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 sudo, so I had a look at the top-level makefile, on which I have several comments.

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 cp and subsequent rm commands are equivalent to mv, so you could just use that. In fact, it'd make more sense to me to do

cd FASMA/models
tar -zxvf apogee_kurucz.tar.gz
tar -zxvf marcs.tar.gz
cd ../..

With the pip install listed in an instructions page, the user can decide whether to use pip install with or without the --user flag or sudo.

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.

[warrickball]

@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 pip install FASMA

I'm not familiar with everything that setup.py can do but if it can handle extracting the atmosphere tables and perhaps even installing MOOG, then I agree that's the best way to handle installation (which would override my remarks above).

[ipelupessy]

@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.

[ipelupessy]

some comments on running:

• the configuration with StarMe_synth.cfg is not very intuitive; it seems to need a particular formatting; it would be better if you could provide these through python keyword arguments directly
• I made a cfg file with just the following:

  # this is a header (all special characters will act as header/comments)
  # linelist teff logg [M/H] vt vmac vsini options
  giraffe_sun_arcturus_calib.lst

  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 https://github.com/MariaTsantaki/FASMA-synthesis/pull/13 ). turns out the default suggested by the documentation is not correct (intervals_hr10_15n.lst)

• after this MOOG, fasma complained about missing summary.out. It turns out MOOG was giving an error about missing data. By default the output of MOOG is hidden though, so you would not know. This is not good behaviour - you can hide the errors, but then you should catch and act on them, or just report the output of MOOG (PR https://github.com/MariaTsantaki/FASMA-synthesis/pull/14)
• with the data (Barklem.dat) installed in the proper location, fasma runs and finishes. The output files synthresults.dat and synthresults_elements.dat are empty apart from the header. I am not sure this is an error of some sort or expected behaviour...
• fasma seems to be made to run in a particular location; for this sort of tool you want to be able to run it in some seperate project folder...(I ran in a test folder, I had to link everything through). It would be better if the needed file locations are pulled from the installation dirs.

[ipelupessy]

@MariaTsantaki some further comments: for this kind of wrapping it is nice if you can continue from within python with the results - so like: result = fasma(**kwargs) with kwargs the options as keyword arguments (as mentioned above) and result a python object encapsulating the output with some metadata. Now you are going to have to parse data files in some obscure format...

[ipelupessy]

https://github.com/MariaTsantaki/FASMA-synthesis/pull/15 contains the above mentioned stub for sm

[MariaTsantaki]

Hi all, Thank you for the comments! They are very useful. I am implementing them and it may take me some time.

[MariaTsantaki]

Dear reviewers, Thank you very much for the comments. They helped to improve the code. The major changes were on the setup, how FASMA function works in scripts, and the configuration file. This version seems easier to work with.

1) The configuration file (renamed from StarMe.cfg to config.yml) has now a yaml format with all the keys/options available. The file contains the default options.

2) There were some classes renamed and as suggested, now FASMA can accept options and update the default ones. e.g.

from FASMA import FASMA 
options = {'teff':4500, 'fix_teff':True}
result = FASMA(**options)

3) Setting up MOOG is tricky, hopefully now it will work better. The shell script install_fasma.sh will fill the moog path but will ask for the system type. There is a hidden limit for the length of the path characters though for fortran. I commented out the places where sm was used. I checked installed FASMA for versions with and without sudo: pip install . and pip install --user . They both work but I leave it as the first for now.

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!

[warrickball]

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

/home/wball/.local/lib/python3.7/site-packages/pyfits/__init__.py:22: PyFITSDeprecationWarning: PyFITS is deprecated, please use astropy.io.fits
  PyFITSDeprecationWarning)  # noqa
Traceback (most recent call last):
  File "/home/wball/.local/bin/fasma", line 12, in <module>
    driver = FASMA.FASMA(cfgfile=cfgfile, overwrite=None)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 49, in __init__
    self.synthdriver()
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 530, in synthdriver
    for (self.initial, self.options) in self._genStar():
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 124, in _genStar
    self._setup(line)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 104, in _setup
    self._options(line)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 245, in _options
    defaults['snr'] = snr(defaults['observations'])
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 286, in snr
    snr = [sub_snr(snr_region) for snr_region in snr_regions]
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 286, in <listcomp>
    snr = [sub_snr(snr_region) for snr_region in snr_regions]
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 247, in sub_snr
    wave_cut, flux_cut, l = read_observations(fname, w1, w2)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 122, in read_observations
    start_wave = header['CRVAL1']  # initial wavelenght
  File "/home/wball/.local/lib/python3.7/site-packages/astropy/io/fits/header.py", line 148, in __getitem__
    card = self._cards[self._cardindex(key)]
  File "/home/wball/.local/lib/python3.7/site-packages/astropy/io/fits/header.py", line 1708, in _cardindex
    raise KeyError(f"Keyword {keyword!r} not found.")
KeyError: "Keyword 'CRVAL1' not found."

I presume that FASMA expects the spectrum to be in a particular format (that includes the CRVAL1 key in the FITS header) but this isn't documented anywhere.

Here are the contents of hd38529_harps.yml:

$ cat hd38529_harps.yml
# 
star1: 
    linelist: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/linelist.lst 
    teff: 5578
    logg: 3.832
    feh: 0.338
    vt: 1.0
    vmac: 3.21
    vsini: 1.90
    model: apogee_kurucz
    MOOGv: 2014
    save: False
    element: False
    fix_teff: False
    fix_logg: False
    fix_feh: False
    fix_vt: False
    fix_vmac: False
    fix_vsini: False
    plot: True
    plot_res: False
    damping: 1
    step_wave: 0.01
    step_flux: 3.0
    minimize: True
    refine: False
    observations: /home/wball/Downloads/ADP.2018-11-14T01_07_37.242.fits
    intervals_file: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/intervals.lst 
    snr: null
    resolution: 115000
    limb: 0.6

[ipelupessy]

I managed to install and run with the provided instructions. The path limit is annoying; I have made a PR (https://github.com/MariaTsantaki/FASMA-synthesis/pull/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 ??

[ipelupessy]

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.

[ipelupessy]

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.

[ipelupessy]

@xuanxu: I have my final comments out above; if these are adressed I think the checklist is ok. Authors have made very good progress on improving the installation. I think the minor points mentioned above will also be addressed promptly ;-)

[xuanxu]

@ipelupessy Great, thanks! Let's wait for the authors to address them so you can complete the checklist.

[warrickball]

I managed to get FASMA to run renaming the file to .spec and changing these lines in read_observations (observations.py):

            flux = x['FLUX'][0] # x['flux']
            wave = x['WAVE'][0] # x['wavelength']

The run eventually crashes with

Iter       4    CHI-SQUARE =  77689.37109  DOF =  26425
   Teff = 5780.200516  
   logg = 3.08652508  
   [Fe/H] = 0.1202310063  
   vt = 0  
   vmac = 11.79304433  
   vsini = 1.201662041  
    Teff:  5880.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5680.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 3.24   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 2.94   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.22   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.02   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.20   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 12.29   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 11.29   vsini: 1.20
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 2.70
    Teff:  5780.2   logg: 3.09   [Fe/H]: 0.12   vt: 0.00   vmac: 11.79   vsini: 0.00
    Teff:  5769.7   logg: 2.34   [Fe/H]: 0.08   vt: 0.00   vmac: 20.00   vsini: 1.02
Traceback (most recent call last):
  File "/home/wball/.local/bin/fasma", line 12, in <module>
    driver = FASMA.FASMA(cfgfile=cfgfile, overwrite=None)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 49, in __init__
    self.synthdriver()
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 575, in synthdriver
    self.status = self.minizationRunner()
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 383, in minizationRunner
    self.params, self.xo, self.yo = function.minimize()
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/minimization.py", line 349, in minimize
    maxiter=20,
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/mpfit.py", line 1306, in __init__
    [self.status, wa4] = self.call(fcn, self.params, functkw)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/mpfit.py", line 1578, in call
    [status, f] = fcn(x, fjac=fjac, **functkw)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/minimization.py", line 251, in myfunct
    p, atmtype=self.model, driver='synth', ranges=self.ranges, **options
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/utils.py", line 570, in fun_moog_synth
    epsilon=options['limb'],
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthetic.py", line 259, in broadening
    y_mac = vmac_broadening(x, y, vmacro_rad=vmac)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthetic.py", line 240, in vmac_broadening
    ("Spectrum range too narrow for macroturbulent broadening")
ValueError: Spectrum range too narrow for macroturbulent broadening

but I'm not a spectroscopist, so I'm not sure whether this is my fault. The initial parameters I used are a rough average of the values under HD 38529's SIMBAD entry but the fit appears to diverging from these values.

[MariaTsantaki]

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

/home/wball/.local/lib/python3.7/site-packages/pyfits/__init__.py:22: PyFITSDeprecationWarning: PyFITS is deprecated, please use astropy.io.fits
  PyFITSDeprecationWarning)  # noqa
Traceback (most recent call last):
  File "/home/wball/.local/bin/fasma", line 12, in <module>
    driver = FASMA.FASMA(cfgfile=cfgfile, overwrite=None)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 49, in __init__
    self.synthdriver()
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 530, in synthdriver
    for (self.initial, self.options) in self._genStar():
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 124, in _genStar
    self._setup(line)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 104, in _setup
    self._options(line)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/synthDriver.py", line 245, in _options
    defaults['snr'] = snr(defaults['observations'])
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 286, in snr
    snr = [sub_snr(snr_region) for snr_region in snr_regions]
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 286, in <listcomp>
    snr = [sub_snr(snr_region) for snr_region in snr_regions]
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 247, in sub_snr
    wave_cut, flux_cut, l = read_observations(fname, w1, w2)
  File "/home/wball/.local/lib/python3.7/site-packages/FASMA/observations.py", line 122, in read_observations
    start_wave = header['CRVAL1']  # initial wavelenght
  File "/home/wball/.local/lib/python3.7/site-packages/astropy/io/fits/header.py", line 148, in __getitem__
    card = self._cards[self._cardindex(key)]
  File "/home/wball/.local/lib/python3.7/site-packages/astropy/io/fits/header.py", line 1708, in _cardindex
    raise KeyError(f"Keyword {keyword!r} not found.")
KeyError: "Keyword 'CRVAL1' not found."

I presume that FASMA expects the spectrum to be in a particular format (that includes the CRVAL1 key in the FITS header) but this isn't documented anywhere.

That's a very nice catch. It has been some time since ESO has been standardizing their reduced fits files but I haven't been aware of that because I have been using older data. I followed their recommendations to read such files. https://archive.eso.org/cms/eso-data/help/1dspectra.html I push the changes for this.

[MariaTsantaki]

@ipelupessy Thanks again for the comments. I added more information on the Summary for the purpose of this tool and more details for the technical part. I also updated the README and manual. I am not sure how other tests would be useful but I can include more spectra to test more than one star.

[MariaTsantaki]

@warrickball You almost made it! FASMA reached its limit for macroturbulence and it broke. There were not efficient number spectral points for the calculations. Since 20 km/s is too high for 'normal stars' I suspect there is another problem here. The spectrum region can be too narrow if other velocities with similar broadening profiles broaden the lines too much such as rotation. In this case I would recommend to increase the ranges in the intervals.lst file. However, in this case I suspect another problem. I neglected to mention that the input spectra for FASMA have to be corrected for radial velocities (RVs). You can see if it is corrected or not by just plotting the spectrum with FASMA (configure the config.yml file) and see that it is shifted compared to the synthetic one. I have a simple (not quite well written) package to calculate RVs. I can add this in another repository just in case.

[warrickball]

I expect that the file format issues would restrict FASMA to some specific subset of spectra or require the user to modify their data for the command line script to run. It would be better to specify some FASMA data format to separate that from the analysis and define that format somewhere in the documentation. e.g. if I know that FASMA can use a plain text file with columns of wavelength (in some units) versus flux (in some units), then I can deal with converting my spectra to that format and leave FASMA to analyse it. This basically already exists but isn't documented. I looked at the source code to infer the appropriate FITS structure.

[ipelupessy]

..and/or accept a dictionary with the needed data as input for 'observations' ..

[warrickball]

After I shifted the wavelength values in the spectrum by 30 km/s, I got a decent fit to my star, so I think I'm now happy that FASMA is capable of fitting the fundamental parameters of stellar spectra. This did, however, involve writing myself some scripts to convert between various formats and apply the RV correction. Assuming the RV correction is as simple as multiplying by (1+v/c), it would be nice if FASMA could do this for the user.

I'm now testing whether FASMA can determine individual abundances, as said in the paper. I worked out (undocumented) that I needed to change intervals_file from intervals.lst to intervals_elements.lst and linelist from linelist.lst to elements.lst. I got a result for Na, Mg, Sc, Ti, V, Cr and Mn but for Al, Si, Ca and Ni I get errors like this for the Sun:

Line list: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/elements.lst
Intervals list: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/intervals_elements.lst
Linelist contains 70 lines in 2 intervals
SNR: 261
Observed spectrum contains 800 points
Starting minimization for element abundance...
    [Al/H]: 0.00
Traceback (most recent call last):
  File "/home/wball/.local/bin/fasma", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/home/wball/work/FASMA-synthesis/bin/fasma", line 12, in <module>
    driver = FASMA.FASMA(cfgfile=cfgfile, overwrite=None)
  File "/home/wball/work/FASMA-synthesis/FASMA/synthDriver.py", line 49, in __init__
    self.synthdriver()
  File "/home/wball/work/FASMA-synthesis/FASMA/synthDriver.py", line 601, in synthdriver
    self.status = self.minizationElementRunner()
  File "/home/wball/work/FASMA-synthesis/FASMA/synthDriver.py", line 436, in minizationElementRunner
    self.elemabund, self.xo, self.yo = function.minimizeElement()
  File "/home/wball/work/FASMA-synthesis/FASMA/minimization.py", line 389, in minimizeElement
    maxiter=20,
  File "/home/wball/work/FASMA-synthesis/FASMA/mpfit.py", line 1031, in __init__
    [self.status, fvec] = self.call(fcn, self.params, functkw)
  File "/home/wball/work/FASMA-synthesis/FASMA/mpfit.py", line 1578, in call
    [status, f] = fcn(x, fjac=fjac, **functkw)
  File "/home/wball/work/FASMA-synthesis/FASMA/minimization.py", line 254, in myfunct
    sl = InterpolatedUnivariateSpline(xs, ys, k=1)
  File "/home/wball/.local/lib/python3.7/site-packages/scipy/interpolate/fitpack2.py", line 601, in __init__
    raise ValueError('x must be strictly increasing')
ValueError: x must be strictly increasing

Here's my configuration file, mostly adapted from the example:

# 
star1: 
    teff: 5777
    logg: 4.44
    feh: 0.0
    vt: 1.0
    vmac: 3.21
    vsini: 1.90
    model: apogee_kurucz
    MOOGv: 2014
    save: False
    # Na, Mg, Al, Si, Ca, Sc, Ti, V, Cr, Mn, Ni
    element: 'Al'
    fix_teff: False
    fix_logg: False
    fix_feh: False
    fix_vt: False
    fix_vmac: False
    fix_vsini: False
    plot: True
    plot_res: False
    damping: 1
    step_wave: 0.01
    step_flux: 3.0
    minimize: False
    refine: False
    observations: /home/wball/work/FASMA-synthesis/FASMA/spectra/Sun_HARPS.fits 
    intervals_file: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/intervals_elements.lst 
    linelist: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/elements.lst 
    snr: null
    resolution: null
    limb: 0.6

[warrickball]

I've just noticed that the changes to the line data filenames are mentioned on the top-level README. Sorry!

[warrickball]

Now that I could finally install and confirm that the code runs and is reasonably usable, here's my first pass at reviewing it from beginning to end and identifying some outstanding issues. I've ticked off things I'm already satisfied with and commented below on improvements that could be made (including things that are already satisfactory but could be better).

License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?

I'm not a licensing expert but I'm not really sure how to accommodate MOOG's unclear licensing status. I think the best solution would be that the FASMA repo doesn't contain MOOG but FASMA also needs to modify MOOG to remove the SuperMongo dependency. I'm not sure what to suggest but I remain uncomfortable until an expert has weighed in.

Installation: Does installation proceed as outlined in the documentation?

Mostly. I ran the pip install command myself after installation so that I could add the --user flag. It would be nice if the install script would somehow allow arguments like this to be passed and even better if the whole installation could be handled by pip. Or just remove the pip command from the install script and change the documentation to say "run the install script, then pip install FASMA".

Functionality: Have the functional claims of the software been confirmed?

Not quite. I get errors when I try to determine the individual abundances of Al, Si, Ca and Ni in the bundled solar spectrum.

Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

While there aren't any performance claims, there might be places where the code could be sped up. For example, it looks like the observed spectrum is re-read many times while the code is running. This isn't noticeable for FITS files, which are binary, but it was noticeable when reading from a ~115,000 line plain text HARPS spectrum.

Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

The requirements for FASMA's Python package are clear (and I appreciate that there are relatively few). There is no clear list of what MOOG requires but I imagine any but the most ancient of Fortran compilers will do.

Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

The authors include one example of fitting the fundamental properties but not of fitting individual abundances, which for me doesn't currently work for all elements.

Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

At the moment, I find the documentation unsatisfactory. Though there is a PDF manual that lists the options for the configuration file and a top-level README.md with some other details, neither specifies some important details like expected file formats. The README.md has a few lines about using the Python functions directly but I couldn't find any documentation for the Python API. On the other hand, the authors don't motivate using the Python interface instead of the command line program so I'd be satisfied if they just dropped the Python interface example. Otherwise, I expect API documentation somewhere.

I also find it slightly frustrating having to flip back and forth between two distinct sources of documentation: the README.md and the PDF "manual". Ideally there'd be Sphinx documentation rendered somewhere.

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

There don't appear to be any automated tests of the main functionality but the fact that the example runs to completion suffices. If a similar example is created for the individual solar abundances, I would be satisfied.

Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

No.

Software paper A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

I think the value of spectroscopic analysis can be made more clearly, even though the authors do already say it's one of the basic things we rely upon in astronomy. Example subfields in which I can think of spectroscopy having a big influence are in constraining stellar physics and distinguishing stellar populations. Detecting companions (be they stars or planets) also requires spectroscopic analysis but with the radial velocity, which FASMA doesn't handle.

I think it's also worth pointing out that spectroscopic analysis is currently not very accessible. So, though codes like MOOG exist, they are relatively difficult to use. The real value in a code like FASMA is to make the process that much easier. Even though I've never used MOOG directly and FASMA's documentation is only partially complete, it still enabled me to fit a stellar spectrum, which is an achievement!

State of the field: Do the authors describe how this software compares to other commonly-used packages?

The authors don't compare FASMA to other software. I'm not a specialist myself but perhaps Spectroscopy Made Easy is similar? Or CMFGEN (though that might be for hot stars)?

Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?

I would say the paper would benefit from language editing. (To be fair, most papers—including my own—would.)

References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

There's no reference for the implementation of the Levenberg–Marquardt algorithm (currently in mpfit.py). I presumed the authors would use a standard Python package for this (e.g. scipy.optimize.leastsq or lmfit) but this implementation comes from some other (not clearly licensed) source.

[MariaTsantaki]

After I shifted the wavelength values in the spectrum by 30 km/s, I got a decent fit to my star, so I think I'm now happy that FASMA is capable of fitting the fundamental parameters of stellar spectra. This did, however, involve writing myself some scripts to convert between various formats and apply the RV correction. Assuming the RV correction is as simple as multiplying by (1+v/c), it would be nice if FASMA could do this for the user.

I'm now testing whether FASMA can determine individual abundances, as said in the paper. I worked out (undocumented) that I needed to change intervals_file from intervals.lst to intervals_elements.lst and linelist from linelist.lst to elements.lst. I got a result for Na, Mg, Sc, Ti, V, Cr and Mn but for Al, Si, Ca and Ni I get errors like this for the Sun:

Line list: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/elements.lst
Intervals list: /home/wball/work/FASMA-synthesis/FASMA/rawLinelist/intervals_elements.lst
Linelist contains 70 lines in 2 intervals
SNR: 261
Observed spectrum contains 800 points
Starting minimization for element abundance...
    [Al/H]: 0.00
Traceback (most recent call last):
  File "/home/wball/.local/bin/fasma", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/home/wball/work/FASMA-synthesis/bin/fasma", line 12, in <module>
    driver = FASMA.FASMA(cfgfile=cfgfile, overwrite=None)
  File "/home/wball/work/FASMA-synthesis/FASMA/synthDriver.py", line 49, in __init__
    self.synthdriver()
  File "/home/wball/work/FASMA-synthesis/FASMA/synthDriver.py", line 601, in synthdriver
    self.status = self.minizationElementRunner()
  File "/home/wball/work/FASMA-synthesis/FASMA/synthDriver.py", line 436, in minizationElementRunner
    self.elemabund, self.xo, self.yo = function.minimizeElement()
  File "/home/wball/work/FASMA-synthesis/FASMA/minimization.py", line 389, in minimizeElement
    maxiter=20,
  File "/home/wball/work/FASMA-synthesis/FASMA/mpfit.py", line 1031, in __init__
    [self.status, fvec] = self.call(fcn, self.params, functkw)
  File "/home/wball/work/FASMA-synthesis/FASMA/mpfit.py", line 1578, in call
    [status, f] = fcn(x, fjac=fjac, **functkw)
  File "/home/wball/work/FASMA-synthesis/FASMA/minimization.py", line 254, in myfunct
    sl = InterpolatedUnivariateSpline(xs, ys, k=1)
  File "/home/wball/.local/lib/python3.7/site-packages/scipy/interpolate/fitpack2.py", line 601, in __init__
    raise ValueError('x must be strictly increasing')
ValueError: x must be strictly increasing

You are right. There were overlapping wavelength intervals for the synthesis and the interpolation was crashing. I fixed this in the synthetic.py file when reading the intervals now they are merged when there are overlapping ones. Thanks! I also added from VALD some missing lines I noticed from the plots. And I checked now it works properly for the elements.

from FASMA import FASMA

 elements = ['Na', 'Mg', 'Al', 'Si', 'Ca', 'Sc', 'Ti', 'V', 'Cr', 'Mn', 'Ni']

 for el in elements:

     options = {'plot':True, 'linelist':'FASMA-synthesis/FASMA/rawLinelist/elements.lst', 'intervals_file':'FASMA-synthesis/FASMA/rawLinelist/intervals_elements.lst', 'element':el, 'resolution':115000, 'observations': 'FASMA-synthesis/FASMA/spectra/Sun_HARPS.fits'}

        r = FASMA(**options)

[MariaTsantaki]

Now that I could finally install and confirm that the code runs and is reasonably usable, here's my first pass at reviewing it from beginning to end and identifying some outstanding issues. I've ticked off things I'm already satisfied with and commented below on improvements that could be made (including things that are already satisfactory but could be better).

License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?

I'm not a licensing expert but I'm not really sure how to accommodate MOOG's unclear licensing status. I think the best solution would be that the FASMA repo doesn't contain MOOG but FASMA also needs to modify MOOG to remove the SuperMongo dependency. I'm not sure what to suggest but I remain uncomfortable until an expert has weighed in.

I am not sure what to do here too. I prefer to keep it inside FASMA because MOOG versions may change and also the SuperMongo dependency is now removed. I added some information about the MOOG code in its folder but if this is not enough, please let me know on how to proceed.

[MariaTsantaki]

Functionality: Have the functional claims of the software been confirmed?

Not quite. I get errors when I try to determine the individual abundances of Al, Si, Ca and Ni in the bundled solar spectrum.

True. We solved this with a recent commit.

[MariaTsantaki]

@warrickball I will need more time to carefully look the rest of the comments! Thanks!

[xuanxu]

@whedon generate pdf

[whedon]

:point_right: Check article proof :page_facing_up: :point_left:

[MariaTsantaki]

@warrickball About installation: I agree it would be better to control everything with pip but I did not succeed so I spit the installation in 2 steps as it was proposed.

[MariaTsantaki]

@warrickball About Performance: I added a comment for the users to prefer fits rather than text. Also I included instructions about the format of the input files.

About documentation: I understand that the information was split into two places (README and manual) so I merged them. There are a lot of things to improve (I wish I knew sphinx earlier). Now it seems very difficult to compile the current documentation to readthedocs (https://readthedocs.org/projects/fasma-synthesis/). I failed but it is something I want to look in the future. Nevertheless, I updated the documentation.

About Automated tests: I included more in the tests folder about the minimization process. This code is run separately from installation. This is the only way I could think of.

State of the field: There are indeed other packages such as SME (http://www.stsci.edu/~valenti/sme.html) and ispec (https://www.blancocuaresma.com/s/iSpec) that use the spectral synthesis.

References: In fact there is a license for mpfit. It was first written in idl where I have included the reference in the paper. https://pages.physics.wisc.edu/~craigm/idl/fitqa.html

[xuanxu]

@whedon generate pdf

[whedon]

:point_right: Check article proof :page_facing_up: :point_left:

[warrickball]

Hi @MariaTsantaki, I can see you've made big improvements to the package!

• Installation: I'm happy with the two-step installation. I just re-installed FASMA from scratch without a problem.
• Tests: The new test_FASMA.py is a good start and good enough to satisfy me for this review. It's a bit difficult to make sense of all of the output but it ran to completion without any obvious errors so I'm happy with that.
• Paper: I'd still like the paper to mention other spectral synthesis packages and how FASMA differs. Otherwise, it looks fine, although some of the mathematical typesetting is incorrect (e.g. Teff, the χ² equation).
• Documentation: Thanks for combining the sources. The README.md alone is now fine for command-line use (after all, I managed!) and OK for the Python interface. I can see that you're trying to create Sphinx documentation, which is great. I'd say don't worry about fighting to get it working on ReadTheDocs just for this submission. I'm happy for you to just tell users they can go to FASMA/docs and build the documentation with make html. That said, the repo currently looks like you've ended up with two Sphinx setups, one with source in docs/ and another with source in docs/source. I managed to generate documentation by changing SOURCEDIR from source to . in the Makefile. If you're having trouble, I'd suggest wiping docs and starting again. Also, only add to the repo the files that you need to generate the docs, not Sphinx's outputs.

So for me, the outstanding items are:

1. mentioning other packages in the paper (State of the field),
2. create community guidelines (Community guidelines), and
3. the licensing of MOOG (License).

@xuanxu, can you comment on item 3? I don't feel expert enough of licensing issues to know whether the current situation with MOOG is OK. MOOG is bundled but doesn't have an obvious license. Basically, I want someone more knowledgeable than me to say "This is fine" and I'll be happy.

[MariaTsantaki]

@warrickball Thanks a lot! There is some documentation now. I added the 1st (paper) and 2nd (README) point.

[xuanxu]

@warrickball Yes, we should address the MOOG license issue. If we don't know which license MOOG is using (I can't find it in their website) I suggest @MariaTsantaki or any other FASMA's author ask MOOG's author, maybe he can point us to where the license is or tell us if MOOG license allows redistributing. If that is not the case they could ask for explicit written permission for FASMA. Otherwise I guess we should assume copyright and FASMA could not bundle it in their code, adding it as a pre-requisite and including instructions about how to install it.

[MariaTsantaki]

Ok I will send an email to the author.

[MariaTsantaki]

Hi all, The author of MOOG is kindly ok with its redistribution. He mentioned to add a disclaimer on the README that we are not taking responsibility for proper use of MOOG as it is also mentioned in his website which I guess lies under the MIT license. I will ask him if possible to add the license online but he mentioned he is also willing to send a letter to the journal if necessary about this.

[xuanxu]

@MariaTsantaki Great, if he specifies the MIT license online and/or adds the license file to the downloadable .tar.gz file with the code, that would be enough.