-
-
Notifications
You must be signed in to change notification settings - Fork 42
Documentation facelift #275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
+ Coverage 89.52% 89.57% +0.04%
==========================================
Files 17 17
Lines 1795 1803 +8
==========================================
+ Hits 1607 1615 +8
Misses 188 188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
not sure if it fits in this PR or a future one, but we have a lot of example docs implemented within docstrings in various parts of the code (i am one of the guilty parties). as part of a fuller refactor, we should decide whether to leave that as-is, migrate it to specific |
7ee2d16 to
c7344e7
Compare
|
The documentation update is coming together! Please take a look at the rendered docs at No need to worry about the large number of commits and changed files. This PR builds on the 1D wavelength calibration PR, which should be merged first. |
- Added `sphinx-copybutton` and `sphinx-design` to documentation dependencies in `pyproject.toml`. - Improved content structure by renaming and reorganizing documentation files. - Expanded "Contributing" and "Index" sections.
…action examples; updated `conf.py` with formatting adjustments and extended Sphinx documentation settings.
- Corrected typos and clarified text. - Renamed `common.py` to `quickstart.py`. - Updated function and variable names for consistency. - Streamlined background subtraction and wavelength calibration workflows.
…uncertainty handling.
…tdDevUncertainty` when absent.
…doc_xref_param_type` configuration.
c6a96b5 to
d4d0c76
Compare
docs/background.rst
Outdated
| * `Background.one_sided <specreduce.background.Background.one_sided>` | ||
| * `Background.two_sided <specreduce.background.Background.two_sided>` | ||
|
|
||
| The center of the window can either be passed as a float/integer or as a trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specify that trace is a ~specreduce.tracing.Trace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/background.rst
Outdated
|
|
||
| The `specreduce.background` module generates and subtracts a background image from | ||
| the input 2D spectral image. The `~specreduce.background.Background` object is | ||
| defined by one or more windows, and can be generated with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify what 'windows' means in this context (maybe using trace, separation and width in the description)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/contributing.rst
Outdated
| 3. Commit your changes with a clear message describing what you did. | ||
| 4. Open a pull request to the main repository. | ||
|
|
||
| .. If you’re unsure how to start, you can look for issues labeled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncomment this section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to remove it completely for now. I still need to organise the issue labels better.
docs/extraction.rst
Outdated
|
|
||
| Example Workflow | ||
| ================ | ||
| 'n_bins_interpolated_profile': 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing closing brace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Fixed.
docs/extraction.rst
Outdated
| from specreduce.background import Background | ||
| from specreduce.extract import BoxcarExtract | ||
| interp_profile_extraction = extract(spatial_profile={'name': 'interpolated_profile', | ||
| 'interp_degree_interpolated_profile': 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing closing brace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is why my personal style is to put one parameter per line and indent. helps make it clearer what ) or } belongs to what ( or { and when one is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. These two examples are now formatted more neatly.
docs/index.rst
Outdated
| **Contributor's Guide** | ||
| ^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Want to contribute to specreduce? Found a bug? The contributing guidelines will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capitalize 'specreduce'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/index.rst
Outdated
| process/index | ||
| terms | ||
|
|
||
| .. * :ref:`genindex` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncomment/remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| @@ -0,0 +1,166 @@ | |||
| Tracing | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need a reference at the top as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/trace.rst
Outdated
| the 2D spectral image as input, along with trace-specific parameters that control how the trace | ||
| is determined. | ||
|
|
||
| Flat trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FlatTrace, for consistency with other section headings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/trace.rst
Outdated
|
|
||
| .. code-block:: python | ||
| trace = specreduce.tracing.ArrayTrace(image, positions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you define positions in a line of code, or explain what it is above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the text a bit to make it clear.
tepickering
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! i mostly agree with the comments @cshanahan1 left and added a few more, but think they're all pretty minor. the new style looks SOOO much better! there's some more work to do to better organize some parts, but this sets up a much better, more usable foundation for that.
docs/extraction.rst
Outdated
| from specreduce.background import Background | ||
| from specreduce.extract import BoxcarExtract | ||
| interp_profile_extraction = extract(spatial_profile={'name': 'interpolated_profile', | ||
| 'interp_degree_interpolated_profile': 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is why my personal style is to put one parameter per line and indent. helps make it clearer what ) or } belongs to what ( or { and when one is missing.
| conda install conda-forge::specreduce | ||
| or by cloning the repository from `GitHub <https://github.com/astropy/specreduce>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is cloning from the repo something we want normal users to do? my feeling is that's more for a developer install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to remind users that this is an option. It's useful to have it clearly documented for anyone who wants to have the option to examine the code if something doesn't work as expected or as they'd like.
| from specreduce.utils.synth_data import make_2d_arc_image, make_2d_trace_image | ||
|
|
||
|
|
||
| def make_2d_spec_image( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this function was lifted from synth_data and not just updated there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for this was to make minor modifications to the mock spectrum without altering how the original function in synth_data works (this would've been more work, and I didn't feel this sort of change belonged in a documentation update PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fair. i like the addition of the ability to add an airglow spectrum. i'll have a think about how to make the synth data utilities more easily composable.
|
Thanks @cshanahan1 and @tepickering! The PR is now merged :) |
This PR modernizes the documentation by adopting
sphinx_astropy.conf.v2with thepydata-sphinx-theme. It also updates and reorganizes several existing sections and introduces new documentation covering 1D wavelength calibration.