-
Notifications
You must be signed in to change notification settings - Fork 8
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
Documentation Refactor #295
Conversation
abc60c4
to
c3e961c
Compare
c38c5ee
to
152af91
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This pull request is going to be massive. But the content will not change, so reviewing each file is not important. So for reviews it is much better to focus on the RTD built version to see how it looks and how it flows. |
486c0d5
to
73bb83a
Compare
86d1aeb
to
e51935f
Compare
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.
LGTM. Just two nitpicks on visualization in the gallery examples.
examples/sorting_data.py
Outdated
|
||
fig = plt.figure() | ||
ax = fig.add_subplot(projection=xrt_seq.maps[0]) | ||
ani = xrt_seq.plot(axes=ax) |
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.
For all of these animations, the normalization should be specified (e.g. https://docs.sunpy.org/en/stable/generated/gallery/map/difference_images.html#sphx-glr-generated-gallery-map-difference-images-py) explicitly. Otherwise, it changes from frame to frame which makes it near impossible to actually understand what is being animated.
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.
Should be fixed now with the new norm.
examples/remove_lightleak.py
Outdated
xrt_map.plot(axes=ax, title="Original", clip_interval=(1, 99.9) * u.percent) | ||
ax1 = fig.add_subplot(122, projection=lightleak_map) | ||
lightleak_map.plot( | ||
axes=ax1, title="Light Leak Subtracted", clip_interval=(1, 99.9) * u.percent |
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 would recommend using the same normalization in each plot. Otherwise, it is hard to see what the differences between the two plots actually is.
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.
Should be fixed now with the new image.
@@ -7,7 +7,7 @@ requires = [ | |||
] | |||
|
|||
[project] | |||
name = "xrtpy" | |||
name = "XRTpy" |
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.
Not sure if this is correct.
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 do have a tendency of keeping/using it capitalized
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 mean that one would have to do pip install XRTpy
on case sensitive OSes?
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.
No for pip, this is case insensitive.
The project name must consist of ASCII letters, digits, underscores “_”, hyphens “-” and periods “.”. It must not start or end with an underscore, hyphen or period.
Comparison of project names is case insensitive and treats arbitrarily long runs of underscores, hyphens, and/or periods as equal. For example, if you register a project named cool-stuff, users will be able to download it or declare a dependency on it using any of the following spellings: Cool-Stuff, cool.stuff, COOL_STUFF, CoOl__-.-__sTuFF.
8c6cd09
to
828fa92
Compare
The built version is located at https://xrtpy--295.org.readthedocs.build/en/295/ |
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 have thoroughly reviewed the changes and confirmed that the updates to the documentation structure, including the revised installation steps, API pages, and integration of Sphinx gallery examples, are excellent. The overall structure is cohesive and well-organized. Thank you for the hard work and meticulous effort, @nabobalis !
@@ -48,12 +49,6 @@ repos: | |||
- id: pretty-format-yaml | |||
args: [--autofix] | |||
|
|||
- repo: https://github.com/MarcoGorelli/absolufy-imports |
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.
Replaced by ruff
My goal here is to refactor the entire documentation structure to bring it inline with how its done in sunpy.
This means: