search

openjournals / joss-reviews

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

[REVIEW]: Bayesian X-ray Analysis (BXA) v4.0 #3045

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @JohannesBuchner (Johannes Buchner) Repository: https://github.com/JohannesBuchner/BXA Version: v4.0.5 Editor: @jgostick Reviewers: @cescalara, @cescalara, @grburgess Archive: 10.5281/zenodo.4721575

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@cescalara & @grburgessas & @cescalara, 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 @jgostick 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 ✨

Review checklist for @cescalara

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @grburgess

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @cescalara, @grburgessas, @cescalara it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

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

watching

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

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 commented 3 years ago

PDF failed to compile for issue #3045 with the following error:

Can't find any papers to compile :-(

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=3.56 s (25.0 files/s, 110291.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JSON                            12              0              0         383161
Python                          42           1052           1203           5275
reStructuredText                23            480            224            798
YAML                             2             13             18            208
make                             2             27              6             88
Bourne Shell                     6             24             13             56
XML                              1              9              0             20
Dockerfile                       1             11              1             14
-------------------------------------------------------------------------------
SUM:                            89           1616           1465         389620
-------------------------------------------------------------------------------

Statistical information for the repository '6d3c2cc36ec8f67b7ff24fde' was
gathered on 2021/02/18.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Alberto Masini                   1             2              2            0.02
Brian Refsdal                    8          3223            305           13.24
Johannes Buchner               143         16817           6303           86.73
Liu Teng                         1             1              1            0.01
ruizca                           1             1              1            0.01

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Alberto Masini                2          100.0          0.0                0.00
Brian Refsdal                 5            0.2        121.8               20.00
Johannes Buchner           7523           44.7         43.0               10.71
Liu Teng                      1          100.0          3.3                0.00
grburgess commented 3 years ago

I believe my handle is incorrect should be @grburgess

jgostick commented 3 years ago

Ooops! I'll see if I can get whedon to fix that

jgostick commented 3 years ago

@whedon remove @grburgessas as reivewer

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
jgostick commented 3 years ago

@whedon remove @grburgessas as reviewer

whedon commented 3 years ago

OK, @grburgessas is no longer a reviewer

jgostick commented 3 years ago

@whedon add @grburgess as reviewer

whedon commented 3 years ago

OK, @grburgess is now a reviewer

jgostick commented 3 years ago

@whedon generate pdf from branch joss-paper

JohannesBuchner commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

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

whedon commented 3 years ago

:wave: @cescalara, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @cescalara, please update us on how your review is going (this is an automated reminder).

cescalara commented 3 years ago

Hi guys, I have started the review today and should be able to complete it in the next days.

cescalara commented 3 years ago

Hi @JohannesBuchner, Thanks for all your work on BXA! Overall it looks like things are working and well-documented, but I do have a few comments/questions. I have opened a couple of issues dealing with code-related things:

And also a few comments on the paper:

I really hope my comments are helpful and I'm happy to discuss on any of the points.

Cheers!

JohannesBuchner commented 3 years ago

And also a few comments on the paper:

* What is the relationship to [pyblocxs](https://github.com/brefsdal/pyblocxs)? I realise that this package is no longer active, and little of the original code is left. however, if this code contributed to the ideas and development of BXA, it seems proper to note this and acknowledge the author in the spirit of good open-source etiquette.

Good idea. I used their code to understand how to hook into sherpa, but as you say, little of that is left. I added a line in the acknowledgements.

* For me, it would be useful to have a high-level description of Xspec/sherpa and their differences in the paper. It would also make sense to mention [3ML](https://github.com/threeML/threeML) as an alternative tool for X-ray analysis and the differences there.

There are a bunch of environments, including Spex, ISIS, 3ML, Xspec, sherpa, and probably others (it is hard to do a full survey). It is possible to do meaningful spectral analysis with any of them, they do not differ fundamentally (only in defaults, bells and whistles, models and programming language).

* Would be nice to have a short overview of the methods/functionality in the paper in addition to pointing to your 2014 paper for details.

I am a bit unsure if it is possible to explain all of the concepts (Evidence estimation, Bayes factors, dealing with posterior chains) and their caveats within the expected word limit. I added some text that describes what functionality is included (taken from the readme file). I hope this addresses this.

I really hope my comments are helpful and I'm happy to discuss on any of the points.

They are!

JohannesBuchner commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

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

JohannesBuchner commented 3 years ago

@jgostick I read the COI guidelines, and wanted to mention that @grburgess is employed at the same institute as me. None of the other COI criteria apply, however (no joint papers, funded work, etc.). Not sure if it is a real issue, but I wanted to bring it to your attention for the sake of transparency.

cescalara commented 3 years ago

There are a bunch of environments, including Spex, ISIS, 3ML, Xspec, sherpa, and probably others (it is hard to do a full survey). It is possible to do meaningful spectral analysis with any of them, they do not differ fundamentally (only in defaults, bells and whistles, models and programming language).

Sure, A full survey of what's out there isn't easy. Perhaps just list the tools you mentioned (I wasn't aware of Spex/ISIS) without going into details, and highlight what separates BXA. As far as I can tell, BXA is cool as it is an interface to Bayesian analysis tools that you can easily apply to existing analyses in the very widely used Xspec and sherpa frameworks. So, you get lots of nice features without having to learn a whole new framework. I'm mainly asking this to check off the "State of the field: Do the authors describe how this software compares to other commonly-used packages?" box.

I am a bit unsure if it is possible to explain all of the concepts (Evidence estimation, Bayes factors, dealing with posterior chains) and their caveats within the expected word limit. I added some text that describes what functionality is included (taken from the readme file). I hope this addresses this.

Yup, looks good. It is nice to have an overview of the main functionality.

grburgess commented 3 years ago

At the moment, I am unable to get Sherpa to install in a clean virtual environment with python 3.8 on MacOS.

And pip.

It is also not possible to install XSPEC in a non system-breaking way with homebrew.

@cescalara how were you able to work around this?

grburgess commented 3 years ago

Are there notes somewhere for how to install Sherpa + CIAO on Mac OS? @cescalara which platforms did you use?

cescalara commented 3 years ago

Hi @grburgess,

I tested the installation on Ubuntu in a docker env and followed the instructions linked to in the docs.

I did not yet have time to test on MacOS, but I want to do so.

Some comments for @JohannesBuchner:

JOSS review criteria for reference:

Installation instructions

There should be a clearly-stated list of dependencies. Ideally these should be handled with an automated package management solution.

Good: A package management file such as a Gemfile or package.json or equivalent OK: A list of dependencies to install Bad (not acceptable): Reliance on other software not listed by the authors

grburgess commented 3 years ago

@cescalara aha. Ok, I am currently also installing everything in a linux environment so I hope to be able to proceed from there. I have experience with XSPEC in both linux and macos environments and will only be able to test the XSPEC version within linux. XSPEC now has a docker... but this of course means linux only.

I agree, the sherpa install is very confusing and the capabilities required by BXA via sherpa are unclear at the moment from the documentation. Granted, users familiar with either environment may have the experience to circumvent these issues, it should be documented a bit more clearly what exactly is needed.

Perhaps the pip install could perform some kind of system probe to tell the users what capabilities/features BXA can utilize with their current setup?

cescalara commented 3 years ago

Perhaps the pip install could perform some kind of system probe to tell the users what capabilities/features BXA can utilize with their current setup?

This would certainly be useful.

JohannesBuchner commented 3 years ago

There are four possibilities to have a working ciao/sherpa environment:

Please understand that these are complex programs that are outside of my control. They use non-standard installation methods, and they keep changing every year (and break occasionally). There is no way for me to provide up-to-date installation instructions, or automated installation script. Packaging ciao or heasoft into a docker container or conda package leads to legal problems, as not all of what is contained is FOSS. These options would require me to become a maintainer of these indefinitely and provide trouble-shooting of packages without being able to control the release process.

To give an idea of the difficulty, the heasoft/xspec download file changes URL with every new version, and there is no option other than parsing the HTML of the web page to find out what version is current. Conda is its own nightmare, with environments sometimes unable to resolve dependencies, preventing installation of further packages.

The assumption is that users already use pyxspec or sherpa, and only complement this with BXA, which has the role of a plugin.

The xspec models are not required for BXA with sherpa, although the examples use them.

Perhaps the pip install could perform some kind of system probe to tell the users what capabilities/features BXA can utilize with their current setup?

I am not sure what is meant here. If you have sherpa (as tested by running the sherpa command), you can use bxa.sherpa, if you have pyxspec (as tested with import xspec in python), you can use bxa.xspec.

I am certainly happy to make it as easy as possible for users (improve install docs, provide tools). But I am unsure how I can do it in a sustainable manner.

grburgess commented 3 years ago

I've listed a few issues with getting started here:

https://github.com/JohannesBuchner/BXA/issues/21 https://github.com/JohannesBuchner/BXA/issues/22

Currently, running examples is available only on linux and require XSPEC. Thus testing a Sherpa only install appears impossible, but I am perhaps missing something.

grburgess commented 3 years ago

As the XSPEC install / examples are working on linux I've completed the basic review of this part of the code and documentation:

#23 #24 #25

grburgess commented 3 years ago

@JohannesBuchner

Please understand that these are complex programs that are outside of my control. They use non-standard installation methods, and they keep changing every year (and break occasionally). There is no way for me to provide up-to-date installation instructions, or automated installation script. Packaging ciao or heasoft into a docker container or conda package leads to legal problems, as not all of what is contained is FOSS. These options would require me to become a maintainer of these indefinitely and provide trouble-shooting of packages without being able to control the release process.

To give an idea of the difficulty, the heasoft/xspec download file changes URL with every new version, and there is no option other than parsing the HTML of the web page to find out what version is current. Conda is its own nightmare, with environments sometimes unable to resolve dependencies, preventing installation of further packages.

It is understood, and there is no way to put the burden of maintenance of an enhancement software. Have you communicated with the maintainers of these softwares for the possibility of releasing dockers or some other work around? I've seen statements by the XSPEC maintainers that hint at them not caring too much what is done with the code. But you are providing an enhancement and service to their codes which would multiply their user base. So it would seem logical that they could work with you to ease the access to new users. Of course Conda is always an option and that eases many of the issues. Perhaps (as stated in some of the issues) it is better to inform users of BXA what the current platform limitations are?

I am not sure what is meant here. If you have sherpa (as tested by running the sherpa command), you can use bxa.sherpa, if you have pyxspec (as tested with import xspec in python), you can use bxa.xspec.

In the documentation you have stated to first have the required analysis software installed. Perhaps you can run a script upon install that does some test imports and loudly informs the user that XXX software is not available and will BXA will not function without them. Of course, an experienced user can and will know this ahead of time, I'm more concerned with e.g. a student who wants to do nested sampling, finds BXA and gets frustrated with things not working on the first try. BXA does a good job warning the user about the fitting process:

Null hypothesis probability of 0.000000e+00 with 795 degrees of freedom
 Current data and model not fit yet.
  jeffreys prior for nH between 1.000000e-02 and 1.000000e+03
  jeffreys prior for norm between 1.000000e-10 and 1.000000e+01
   note: this parameter spans *many* dex. Double-check the limits are reasonable.

I think it would ease some pain by being verbose on the install.

JohannesBuchner commented 3 years ago

It is understood, and there is no way to put the burden of maintenance of an enhancement software. Have you communicated with the maintainers of these softwares for the possibility of releasing dockers or some other work around?

I have (with both packages), but without success.

I am not sure what is meant here. If you have sherpa (as tested by running the sherpa command), you can use bxa.sherpa, if you have pyxspec (as tested with import xspec in python), you can use bxa.xspec.

In the documentation you have stated to first have the required analysis software installed. Perhaps you can run a script upon install that does some test imports and loudly informs the user that XXX software is not available and will BXA will not function without them. I think it would ease some pain by being verbose on the install.

Added by commit https://github.com/JohannesBuchner/BXA/commit/2c9d037827e6e837e24656699dafd49a9471074f (included in v4.0.4)

JohannesBuchner commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

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

JohannesBuchner commented 3 years ago

I have revised the submission, and gave responses in the issues opened and mentioned above. @cescalara & @grburgess, please have another look and let me know of any remaining issues.

jgostick commented 3 years ago

Here is a list of what to do next:

grburgess commented 3 years ago

I think many of the issues were addressed, but some of them are still open. I have not had a chance to reinstall everything.

I still think it would be nice to have a badge in the repo detailing which operating systems BXA does work on as I was never able to get its major dependencies installed on Mac OS. This could be handles through CI badging and testing. This would help to differentiate the software as be "feature complete" and not

designed for maintainable extension (not one-off modifications of existing tools). “Minor utility” packages, including “thin” API clients, and single-function packages are not acceptable.

as stated in the review checklist. Currently, BXA adds a different method of obtaining posterior samples to two x-ray analysis packages which already provide a similar functionality as well as some enhanced plotting. But it completely relies on those packages being available for install on a given platform. I'm not sure what the editorial (@jgostick ) opinion is on this, so I'll just leave it linked here: (https://github.com/JohannesBuchner/BXA/issues/22) It is also related to the CI and testing issue but I think these both relate to the same point.

Also, a few of the claims in the paper should be cited or checked regarding the comparison to the included posterior estimation in the core packages. The nested sampling algorithm that BXA attaches to XSPEC and Sherpa, also has tuning parameters (e.g. dlogz, frac_remain etc.) as well as initialization tuning (e.g. warm starts . This will help those reading the paper to understand that these are different methods and one is not actually an improvement on the the other unless there are citations that can show that, for example, MCMC does not work in these situations, though this is not my own personal experience in general. I believe this exists for the affine-invariant flavors of MCMC, but it is not a general statement about MCMC. Moreover, as there do not exist analytic convergence criteria for nested sampling (as there for for Markov transitions), the statement in the paper can be misleading as there exists no posterior convergence checks for nested sampling other than a stoping criteria on the accuracy of the marginal likelihood. Thus, I think it can be pointed out that BXA attaches a nested sampling algorithm to XSPEC and Sherpa to provide the ability to use that algorithm in addition to what is in available in those packages and point out that nested sampling can estimate the marginal likelihood and explore multimodal posteriors without claims about convergence checks and tuning parameters.

Other than those two points, everything else is satisfied.

cescalara commented 3 years ago

Hi @JohannesBuchner, I had another look and some final comments:

JohannesBuchner commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

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

JohannesBuchner commented 3 years ago

Thank you for the useful comments. I address the two remaining issues below.

Regarding which operating systems BXA is supported on:

A section was added to the installation instructions at https://github.com/JohannesBuchner/BXA#installation. It refers the user to supported operating systems.

The tested operating systems of the CI is also mentioned there now (currently only Ubuntu Linux).

I am sorry that the installation of ciao and heasoft on MacOS proves difficult.

Regarding the differentiation of MCMC and NS:

Also, a few of the claims in the paper should be cited or checked regarding the comparison to the included posterior estimation in the core packages. The nested sampling algorithm that BXA attaches to XSPEC and Sherpa, also has tuning parameters (e.g. dlogz, frac_remain etc.) as well as initialization tuning (e.g. warm starts . This will help those reading the paper to understand that these are different methods and one is not actually an improvement on the the other unless there are citations that can show that, for example, MCMC does not work in these situations, though this is not my own personal experience in general. I believe this exists for the affine-invariant flavors of MCMC, but it is not a general statement about MCMC.

I am no nested sampling expert, but I imagine some of the algorithm parameters could be tuned to optimise things for a specific application, even if you have picked robust defaults. When working with MCMC packages, I only usually think about tuning parameters should any issues arise in the sampling with default inputs, so the experience isn't massively different?

The text was modified to remove the claims on tuning parameters (the relevant ones for NS would be the number of live points and parameters of the proposal distribution, which in the case of the MLFriends algorithm used here, is the number of bootstrapping rounds, as well as the termination criterion). Instead, the "statement of need" first present MCMC, and then nested sampling as an algorithm with different properties, without making strong/blanket superiority claims. An example reference to support the speed comparison was added.

The initialization is indeed different (MCMC requiring a starting point, NS starting the exploration from prior sampling), so this was kept, as it is a relevant robustness benefit of NS in X-ray spectroscopy. Warm start is an optional UltraNest feature never used by BXA, so it is not relevant to mention.

The text was also modified to be clearer that the statements only refer to the available MCMC implementations, not to all possible MCMC algorithms (for example, HMC with parallel tempering may have different properties).

Moreover, as there do not exist analytic convergence criteria for nested sampling (as there for for Markov transitions), the statement in the paper can be misleading as there exists no posterior convergence checks for nested sampling other than a stoping criteria on the accuracy of the marginal likelihood. Thus, I think it can be pointed out that BXA attaches a nested sampling algorithm to XSPEC and Sherpa to provide the ability to use that algorithm in addition to what is in available in those packages and point out that nested sampling can estimate the marginal likelihood and explore multimodal posteriors without claims about convergence checks and tuning parameters.

I am no nested sampling expert, but I imagine some of the algorithm parameters could be tuned to optimise things for a specific application, even if you have picked robust defaults. When working with MCMC packages, I only usually think about tuning parameters should any issues arise in the sampling with default inputs, so the experience isn't massively different? Additionally, I'm sure things can occasionally go wrong (e.g. the discussion in https://arxiv.org/abs/1804.06406) so it can be good to encourage users to check out the trace plots and have a think about what is going on rather than encourage "black box" usage. But perhaps, I have misinterpreted what you are saying in the paper.

I agree, and discussion of tuning parameters were removed. However, when the computation is stopped (termination) is importantly different in MCMC and NS. In MCMC, one would typically assess the convergence of the chain(s) and decide whether they have been run long enough, and if not, either resume the chains or run anew for much longer. In NS, the algorithm itself converges, in the sense that after some iterations any newly sampled points do not contribute anything to the posterior (as the volume becomes negligible. This is what was meant here.

The other meaning of convergence -- convergence of the posterior sample distribution to the actual posterior distribution -- is also important. Typically in MCMC it is assessed by comparing multiple chains and their sections (e.g., "r-hat"). The nestcheck paper is an important contribution in this direction for NS. A section was added to the paper to advise users of diagnosing poor runs, based on excess variance in the marginal posterior and ln(Z) value. The algorithm implemented by UltraNest is designed to diagnose itself and adjust automatically in a very conservative fashion (https://arxiv.org/abs/1407.5459). Further diagnostics (e.g., https://arxiv.org/abs/2006.03371) are available, but are a topic of current research, and more research and practical experience is needed to recommend them.

The text was modified to remove language encouraging "black box" usage.

The text changes can be seen in this diff: https://github.com/JohannesBuchner/BXA/compare/6df21b8..joss-paper#diff-0403ef06adf405f7b310b4518bd6a3559854f54c61676f676ce9cbfee7172ab6

I appreciate the comments, and please let me know if you have further suggestions.

jgostick commented 3 years ago

@JohannesBuchner I am sorry for being silent on this...the end of term is always a bit overwhelming. Anyway, I need you to address the checklist items that I posted above. Things like doi, version number, etc. Then we can proceed with publishing.

JohannesBuchner commented 3 years ago

Here is a list of what to do next:

* [ ]  Double check authors and affiliations (including ORCIDs)

Thanks!

* [ ]  Make a release of the software with the latest changes from the review and post the version number here. This is the version that will be used in the JOSS paper.

v4.0.5 at https://github.com/JohannesBuchner/BXA/releases/tag/v4.0.5

* [ ]  Archive the release on Zenodo/figshare/etc and post the DOI here.

Release: https://zenodo.org/record/4721575

DOI: 10.5281/zenodo.4721575

* [ ]  Make sure that the title and author list (including ORCIDs) in the archive match those in the JOSS paper.

Done.

jgostick commented 3 years ago

@whedon set 10.5281/zenodo.4721575 as archive