Skip to content

Conversation

@DavidT3
Copy link
Collaborator

@DavidT3 DavidT3 commented Oct 21, 2025

HEASARC notebook review

Critical review criteria

The author of the pull request should make an effort to go through these check points and ensure that their submission satisfies each point - reviewers will also compare to these checklists.

Science review checklist

  • Does using high-energy data make up a significant part of the tutorial?
  • Is there a use case in the introduction that motivates the code?
  • Does the code do what the introduction says it is going to do?
  • Is it scientifically accurate?
  • Have all necessary references to literature been included?

Formatting checklist

  • Did you base your notebook on the HEASARC-tutorials template?
  • Are all sections in the HEASARC-tutorial template included in your notebook?
  • Is the notebook title compact and informative? It will be the name of the notebook on the HEASARC website!
  • Have you populated the notebook front-matter (the metadata at the top of the notebook)?
  • Is the kernel specified in the front-matter (e.g., heasoft, sas, ciao) correct for the notebook?
  • Have you added an entry for your notebook in the *_index.md file for the containing directory?

Tech review checklist

  • Documentation:
    • Is every function documented?
    • Do all code cells have corresponding narratives/comments?
    • Are all code comments of a purely technical nature? All narratives should be in Markdown cells.
    • Did you populate the 'Runtime' section?
  • Notebook execution, error handling, etc.:
    • Does the notebook run end-to-end, out of the box?
    • Are errors handled appropriately, with try/except statements that are narrow in scope?
    • Have warnings been dealt with appropriately, preferably by updating the code to avoid them (i.e., not by simply silencing them)?
  • Efficiency:
    • Is data accessed from the cloud where possible?
    • Is the code parallelized where possible?
    • If the notebook is intended to be scaled up, does it do that efficiently?
    • Is memory usage optimized where possible?
  • Cleanup:
    • Have blocks of code that need to be re-used been turned into functions and placed in the 'global setup'-'function' section?
    • Has unused code been removed (e.g., unused functions and commented-out lines)?
    • Are comment lines wrapped so all fit within a max of 90 - 100 characters per line?
    • Do plots use color-blind friendly palettes for plotting? Try this simulator for a visual check.

…s CI/CD checks. Now to add more commentary and do some editing. For issue #115
…m plot), but made a fair few changes to the commentary around all the code cells. For issue #115
…ade the major tick formatter of the spectrum x-axis the same as the minor formatter. Also also fixed a stupid mistake where I tried to import matplotlib.plot rather than matplotlib.pyplot (honestly...). For issue #115
…ht curve figure(s). This may move issue #115 to review, after testing
…CER timing guide to additional resources. For issue #115
@DavidT3 DavidT3 linked an issue Oct 21, 2025 that may be closed by this pull request
@DavidT3
Copy link
Collaborator Author

DavidT3 commented Oct 21, 2025

Failure on CircleCI may be to do with the Scorpeon background model - so I suspect that this is the first instance of the XSPEC models directory not being available in the Fornax-Hea image causing issues.

Should be easy to sort, I'll set up a cache for XSPEC models in the same way I have done for calibration data.

DavidT3 and others added 14 commits October 21, 2025 13:37
…run on CircleCI (though does on Fornax proper). I think this is because of the lack of some XSPEC model files. The CircleCI config has been altered to download the right version of those files for the current heasoft, and store them in a cache. They should also be restored from the cache on subsequent runs.
…CircleCI so I can deal with these XSPEC model files I'm downloading. For PR #116
…g installed in the next step (actually running it) fails... For issue #116
…n it does work on Fornax proper. Added some debug ls-es to try and see whether the xspec data are set up properly
…notebook on CircleCI. For PR #116. Upped the chatter of the failing task to get some more info.
…ircleCI (now read and print the XSPEC log file).
@DavidT3
Copy link
Collaborator Author

DavidT3 commented Oct 22, 2025

The cause of my continued problems is still to do with the XSPEC model data - it is now successfully downloaded and linked, but I think my 'clever' way of finding the correct version of xspec-data to download (dry run of conda install) was giving the wrong version of xspec-data because I wasn't running it in the right environment.

That then causes issues like this:

Cannot find any APEC files corresponding to 3.1.2
continuing to use those from 3.1.2

Reading APEC data from 3.1.2

Failed to read continuum data from 
Failed to read 
***calcMultiTempPlasma : failure in calcCIESpectrum, status = 1
Cannot find any APEC files corresponding to 3.1.2
continuing to use those from 3.1.2

When nicerl3-spect tries to generate a background spectrum from the SCORPEON model.

…ok on CircleCI - the method I have for determining the version of model files to download was being run in the wrong environment, so was just retrieving the latest version of xspec-data. For PR #116
…ctral generation cell in the NICER notebook. For issue #115, PR #116
… CircleCI. Should help solve the problems I'm seeing with PR #116
… CircleCI. Should help solve the problems I'm seeing with PR #116
@DavidT3 DavidT3 requested a review from zoghbi-a October 22, 2025 16:21
@DavidT3 DavidT3 self-assigned this Oct 22, 2025
@DavidT3 DavidT3 added doc-content Changes or additions to the content of the documentation doc-infrastructure Changes or additions to the infrastructure to build and serve tutorials and documentation mission-specific Issues that relate to a single high-energy mission ready-for-review HEASARC internal review process can begin labels Oct 22, 2025
from matplotlib.ticker import FuncFormatter
```

## Global Setup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this globally:

# raise errors if a heasoftpy task fails; Remove once this becomes a default in heasoftpy
hsp.Config.allow_failure = False

```{code-cell} python
:tags: [hide-input]

# This cell will be automatically collapsed when the notebook is rendered, which helps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this text?


# NICER ObsID that we will use for this example.
OBS_ID = "4020180445"
SRC_NAME = "PSR-B0833-45"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

referenced only once. Is this really needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this name is not resolvable. Change to: 'PSR B0833-45' (no dash)

SRC_NAME = "PSR-B0833-45"

# The name of the HEASARC table that logs all NICER observations
HEASARC_TABLE_NAME = "nicermastr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user does not generally know this name. Better to find it.

HEASARC_TABLE_NAME = Heasarc.list_catalogs(keywords='nicer', master=True)['name'][0]

:tags: [hide-input]

# Set up the path of the directory into which we will download NICER data
if os.path.exists("../../../_data"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this is for the CI?
It would be great if we can hide this stuff from a user running the notebook interactively.

)
```

```{error}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we just set evtsuffix ourselves in nicerl2 and other tasks and avoid this part. We can say (in a tip or a warning) we are doing this because there is a bug in heasoft 6.36, and that it is generally not needed.

bkgmodeltype="scorpeon",
clobber=True,
noprompt=True,
allow_failure=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do it globally

filtcolumns="NICERV5",
clobber=True,
noprompt=True,
allow_failure=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do this part globally

lcfile="lc.fits",
clobber=True,
noprompt=True,
allow_failure=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do this globally

xs.Fit.perform()

# Read out the plotting information for spectrum and model.
xs.Plot("lda")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expand lda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-content Changes or additions to the content of the documentation doc-infrastructure Changes or additions to the infrastructure to build and serve tutorials and documentation mission-specific Issues that relate to a single high-energy mission ready-for-review HEASARC internal review process can begin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ingest the SciServer NICER example notebook into HEASARC-tutorials

3 participants