Plot vs z#39
Conversation
steven-murray
added
the
type: feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
=======================================
Coverage ? 87.30%
=======================================
Files ? 12
Lines ? 898
Branches ? 0
=======================================
Hits ? 784
Misses ? 114
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bhazelton
left a comment
bhazelton
left a comment
There was a problem hiding this comment.
I haven't actually run the plotting code yet, but here are a few things I noticed on my first read through.
| @@ -12,17 +12,28 @@ | |||
| }, | |||
| { | |||
| "cell_type": "code", | |||
| "execution_count": null, | |||
| "execution_count": 1, | |||
There was a problem hiding this comment.
I think we want to clear outputs?
There was a problem hiding this comment.
Hmmm maybe @JitenDhandha can say more about this. Usually I keep the outputs for notebooks because building the docs doesn't run the code, but maybe we're doing it differently here?
There was a problem hiding this comment.
nbsphinx has a nbsphinx_execute setting that defaults to auto, which means that the notebook is executed when no outputs are present, and skips execution when outputs are present. So I always leave them unexecuted. Easy enough to override or make explicit in conf.py with nbsphinx_execute = "always" or whatever.
There was a problem hiding this comment.
Ah! cool, I don't think that was an option when I originally started using nbsphinx :-)
| "ipykernel>=7.3.0", | ||
| "pip>=26.1.2", |
There was a problem hiding this comment.
why are these being added to the dev group? Surely pip is installed already since uv uses it. And why ipykernel vs jupyter?
There was a problem hiding this comment.
uv actually does not use pip so it needs to be installed explicitly if you want it. I added these so that I could use the uv environment as a jupyter kernel in VSCode. I wasn't entirely sure if this is a good idea to add here. They are dev requirements, but only because I was developing in a particular modality. I can remove them if we think that's better.
|
Hi @steven-murray! Thanks for adding this functionality. I'm leaving a few comments off the top of my head for now:
I'll take a closer look later today or tomorrow as time permits! |
for more information, see https://pre-commit.ci
- Add legend_labeler support for plot-vs-k - Use key-to-label dicts for CLI-friendly legend labels - Add library and CLI tests for legend labeling
- Align shared plot-vs-k and plot-vs-z options - Add fig_width, leg_cols, and colorbar support to plot-vs-k - Remove unused theory_redshifts from plot-vs-z - Add legend label mapping and matching CLI coverage
- Match plotting docstring parameter order to function signatures - Align shared plot-vs-k and plot-vs-z parameter wording - Rename leg_cols to legend_ncols
- rename plot_limits and plot_theories to be k-specific - fix the logic around nz_for_lines - pop kwargs that throw error on plotting for basic styling
- improve phrasing in README - change png name to make clear which one it is - remove custom plotting in tutorial, and polish wording
for more information, see https://pre-commit.ci
- renamed colorbar -> show_colorbar - added color_by option - added functionality for k_label in legend or title
4 participants
This adds a new plotting function,
plot_vs_z.I also added two examples of using this function in the tutorial notebook.