Skip to content

Conversation

@DavidT3
Copy link
Collaborator

@DavidT3 DavidT3 commented Nov 19, 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.

… very similar to the notebook on existing RXTE spectra. For issue #124
…ction to get instrument, ObsID, etc. from the LC file names. For issue #124
…ut the re-processing and generating new light curves section. For issue #124
…notebook, starting to get the actual LC generation to work properly. FOr issue #124
…y spectral information. Adjusting the functions and notebook accordingly. Will likely include a 'here is how you generate high time res LCs' and 'here is how you generate custom energy band lcs' as two separate parts. For issue #124
… into one part that generates custom energy-bound LCs, and another that generates the high time resolution LCs. For issue #124
…annels for the RXTE light curve notebook. For issue #124
…_band_obs functions in the analyze-rxte-lightcurves.md notebook. For issue #124
…) of the analyze-rxte-lightcurves.md notebook. For issue #124
…n the valid observation table. Also added a fair amount of commentary to the part of the notebook where we declare XGA light curve instances for all archived light curves. All in analyze-rxte-lightcurves.md, for issue #124
…e load archival light curves into XGA objects and then into aggregate light curves. Getting bored of this notebook now. For issue #124
… specific time intervals to the analyze-rxte-lightcurves.md notebook. For issue #124
…be too much, I'm drifting from the point of the notebook a little bit). For issue #124
…nalyze-rxte-lightcurves.md notebook. For issue #124
…n) of Section 4 of the analyze-rxte-lightcurves.md notebook. For issue #124
…k that apply CWT peak finding to the whole aggregated light curve. For issue #124
…ghtcurves.md, unfortunately added some extra subsections that I need to fill in, but it'll be worth it. For issue #124
…the analyze-rxte-lightcurves.md notebook. For issue #124
…This may be getting out of hand and I'll have to do some pruning later, but I was exploring the most effective way to communicate whether the average hardness of identified peaks changes with observation time. For issue #124
…in the calculation of peak detection frequency per time chunk. For issue #124
…gure in analyze-rxte-lightcurves.md. For issue #124
…st times section of analyze-rxte-lightcurves.md. Also removed some plots and improved the three panel figure. For issue #124
…section where we examine the burst frequency. For issue #124
@DavidT3 DavidT3 self-assigned this Nov 19, 2025
@DavidT3 DavidT3 added the doc-content Changes or additions to the content of the documentation label Nov 19, 2025
@DavidT3 DavidT3 linked an issue Nov 19, 2025 that may be closed by this pull request
@DavidT3 DavidT3 added mission-specific Issues that relate to a single high-energy mission skip-doc-build Do not trigger automatic CI/CD building of documentation for this PR. labels Nov 19, 2025
@DavidT3
Copy link
Collaborator Author

DavidT3 commented Nov 19, 2025

Skipped the first doc build as I forgot that I need to release a new version of XGA with all the light curve additions.

@DavidT3 DavidT3 removed the skip-doc-build Do not trigger automatic CI/CD building of documentation for this PR. label Nov 19, 2025
… CircleCI as I have done a new release of XGA. For issue #124 and PR #136
…on of the config.yml, same as the Swift-XRT branch. Still need to come up with a better way of doing this.
…st hardness vs count rate figure. Caused the build to fail D: For issue #124 and PR #136
…-lightcurves.md notebook times out. For issue #124
…nfiguration, just trying to get it to pass build without timing out. For issue #124 and PR #136
…ion in the Swift-XRT branch. Need to get better at not fixing unrelated-to-main-focus problems in notebook branches. For PR #136
@DavidT3 DavidT3 requested review from trjaffe and zoghbi-a November 20, 2025 15:27
@DavidT3
Copy link
Collaborator Author

DavidT3 commented Nov 20, 2025

@zoghbi-a @trjaffe - if either of you fancy giving this new notebook a review, that would be great (no worries if you're too busy though).

Clicking the 'All checks have passed' tab at the bottom of the PR page, and then clicking the 'check rendered docs here' link, and finally going to the RXTE light curve notebook page, will get you to the rendered version.

I may have lost focus on this one a bit, so fresh eyes would be great

@zoghbi-a
Copy link
Contributor

This is a very valuable notebook, with lots of details. My high level comments:

  • I think it is long. Maintaining this may be a challenge. It can probably be broken into 3 or 4 notebooks: 1- standard products from the archive, 2- extracting new products, 3- XGA's AggregateLightCurve and LightCurve (which are cool btw), and potentially. 4- the final analysis.
    3 is generic that can be useful for other missions too.

  • On using XGA. Has the code been cross-verified? especially when it comes to the nuances of the fast timing light curves (e.g. GTI effects, barycentering, background etc). If we ask users to use it, we want to be sure it produces results that has been verified. Stingray is an option that has wider community support. I think it may be less of an issue if it is presented in a separate notebook as a 'community-software' section somewhere (similar to what we had for jdaviz and js9 etc).

  • A general note. we should think more about the environment of the notebook. Do we need a conda yaml file? How do we handle multiple notebooks that may have conflicting dependencies.

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 mission-specific Issues that relate to a single high-energy mission

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ingest Tess' RXTE light curve notebook from SciServer cookbooks

3 participants