-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
DOC: Add plot options API examples for Imviz #1704
DOC: Add plot options API examples for Imviz #1704
Conversation
^^^^^^^^^^^^ | ||
|
||
To set the stretch function for just the image being displayed. | ||
The acceptable values are as defined by glue backend:: |
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.
@rosteen , glue vs Glue is back.
Not sure why change log check is cancelled but this PR does not need one. 🤷 |
Codecov ReportBase: 87.16% // Head: 87.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1704 +/- ##
==========================================
+ Coverage 87.16% 87.27% +0.10%
==========================================
Files 95 95
Lines 9914 10097 +183
==========================================
+ Hits 8642 8812 +170
- Misses 1272 1285 +13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
If the goal here is to provide examples for both the astrowidgets and plugin APIs, I think the user might need a little more context (without getting into development details) for the distinction between the two so they don't get confused.... but that is a fine line to try to walk.
How would you feel about moving the plot options examples into a "From the API" section in the plugin docs and link to it from here? This page could then show the astrowidgets API only and include a link that says something like "alternatively, these options can be set from the plot options plugin or its API, which provides the ability to set options across multiple selected viewers/layers simultaneously"?
plot_options.stretch_preset = '95%' | ||
|
||
# Custom | ||
plot_options.stretch_preset = 'Custom' |
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.
technically setting the preset to 'Custom' isn't necessary, but I agree its nice to be verbose here to show the user what is happening.
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 agree; I usually like examples where they show "a little extra"
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
I think this is a better question for @camipacifici or @hcferguson . They would be reading the doc, not me. |
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, thanks.
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.
A small few nitpicks if you wanted to entertain the suggestion. Otherwise, I think it reads fine! Thanks :)
(I still think it would be worthwhile to separate the different APIs and make the distinction more clear, but looks like there are two approvals without that, so we could always reconsider if it seems to cause any user-confusion) |
Co-authored-by: Duy Tuong Nguyen <dtn5ah@virginia.edu>
Description
This pull request is to add user facing examples for plot options API for Imviz so users can operate on multiple images at once.
Fixes #1477
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.