BXA icon indicating copy to clipboard operation
BXA copied to clipboard

ingest David Homan's Bayesian workflow in docs

Open JohannesBuchner opened this issue 2 months ago • 4 comments

PR https://github.com/JohannesBuchner/BXA/pull/51 merged code that shows nice notebooks.

We should

  • add these on the website under "Bayesian workflow" using nb-sphinx, so that they are run when the documentation is built
  • link from the BXA/xspec manual, so that users can switch to it
  • add them to the CI, so that the workflow is run and is ensured to work

JohannesBuchner avatar Apr 25 '24 15:04 JohannesBuchner

@dshoman could you have a look at the following issues, please?

  • Readme should include contact
  • python scripts need pip install natsort which is not yet described in the readme
  • the tutorial_usage_plotxspec.ipynb notebook references files not generated by run_all.sh:
   "epicfn = 'epic_pn_agn_grp1.fak'\n",
    "mos1fn = 'epic_mos1_agn_grp1.fak'\n",
    "mos2fn = 'epic_mos2_agn_grp1.fak'\n",

Maybe the grouping command is missing? Should we go for grouping by 3 (or 5) counts to avoid the biases pointed out in https://giacomov.github.io/Bias-in-profile-poisson-likelihood/ (also referenced in the book chapter)

Otherwise it all ran through on my laptop.

JohannesBuchner avatar Apr 25 '24 20:04 JohannesBuchner

@JohannesBuchner of course, I will add the updates in one new pull request.

  • For the natsort requirement, should this be mentioned in the main readme, or in the readme in the bayesian-workflow directory?

  • The creation of the files "epic_[...]_grp.fak" is part of the commands in gen_xmm.xspec and relies on grppha. So the commands are there, but they are not running correctly on your system. Perhaps is it wrong to assume that grppha is available to everyone running xspec. If so, the easiest solution is probably to simply drop the grouping altogether? I have tested the results on my system and everything also works well without the grouping.

dshoman avatar Apr 26 '24 09:04 dshoman

@JohannesBuchner of course, I will add the updates in one new pull request.

Yes, please.

* For the `natsort` requirement, should this be mentioned in the main readme, or in the readme in the bayesian-workflow directory?

The latter.

* The creation of  the files "epic_[...]_grp.fak" is part of the commands in `gen_xmm.xspec` and relies on `grppha`. So the commands are there, but they are not running correctly on your system. Perhaps is it wrong to assume that `grppha` is available to everyone running `xspec`. If so, the easiest solution is probably to simply drop the grouping altogether? I have tested the results on my system and everything also works well without the grouping.

Ah, sorry. I have ftgrouppha from heasoft, not grppha, but I will see if I should have added something to my configure command during heasoft install. If there is no background, do we need grouping? If there is a background, shouldn't we group by the background spectrum?

JohannesBuchner avatar Apr 26 '24 10:04 JohannesBuchner

Ah, sorry. I have ftgrouppha from heasoft, not grppha, but I will see if I should have added something to my configure command during heasoft install.

For my install, both ftgrouppha and grppha are present. As this is not a given, I would say the safer option would be not to assume they are installed for all users.

If there is no background, do we need grouping? If there is a background, shouldn't we group by the background spectrum?

No, the grouping is not necessary. I left out a background spectrum for simplicity. This could be a nice addition though, to show how this is included in the plotting options. For now, I suggest to leave the grouping (and background) out.

dshoman avatar Apr 26 '24 11:04 dshoman

Please have a look at https://johannesbuchner.github.io/BXA/ for how it looks now and let me know if you want any changes.

JohannesBuchner avatar Apr 29 '24 06:04 JohannesBuchner

Thanks very much for including the notebooks in the documentation, it looks good to me.

There is some unusual output of the PlotBXA notebook, when creating the fakeit spectra (cell 37). There is an XSpec error regarding the file names of the spectra. I am not able to reproduce this on my local systems. It could be OK to simply suppress the output from this cell, as it has no direct input for the rest of the notebook (and as far as I can tell, the simulated files are still generated).

dshoman avatar Apr 29 '24 12:04 dshoman

One thing I don't particularly like in the current navigation on the left is that the titles are so long and there is no clear separation. Bullet points would help. Not sure how to fix that at the moment.

JohannesBuchner avatar Apr 29 '24 14:04 JohannesBuchner

I think the title length should be an easy fix: at the moment only the main header in the notebook (i.e. # ...) is taken into the selection menu. We could simply move the subtitles to a lower level of header (## ... ), that should reduce the entries in the navigation menu to simply 'PlotBXA example' and 'PlotXspec example', respectively.

dshoman avatar Apr 29 '24 14:04 dshoman