Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Initial implementation of abstract class #126

Closed
wants to merge 7 commits into from
Closed

Initial implementation of abstract class #126

wants to merge 7 commits into from

Conversation

pllim
Copy link
Member

@pllim pllim commented Feb 19, 2021 •
edited
Loading

Fix #93

Pre-requisite for #122

Bonus:

TODO

  • Rebase after TST: Stop ignoring warnings #125 is merged
  • Update docs
  • Update example_notebooks
  • How to get Ginga ImageWidget to inherit docstring from BaseImageWidget?
  • See if we can get inheritance diagram to show something saner. If not, just do not show.
  • Get feedback from stakeholders
  • After merge, update all my unmerged PRs here

This was referenced Feb 19, 2021
Closed
Merged
Merged
@pllim
DOC: Update user-facing doc.
0a87db8 
TST: Update test matrix and added a test.

DOC: Re-organized doc and notebooks.

DOC: Update example notebooks
@pllim
TST: Disable oldest deps job
f8082b3
@pllim pllim added the enhancement New feature or request label Feb 19, 2021
@pllim pllim added this to the 0.3 milestone Feb 19, 2021
@pllim

This comment has been minimized.

Sign in to view
@pllim
Copy link
Member Author

pllim commented Feb 19, 2021

Note to reviewers: I don't expect you to go through all that diffs. This is probably the page you want to focus on: https://astrowidgets--126.org.readthedocs.build/en/126/abstract.html

@mwcraig , also note the API changes reflected in the example notebooks diff.

@ejeschke
Copy link
Contributor

@pllim, are any of 'pycairo', 'pillow', 'opencv' or 'aggdraw' installed in the test environment? One of them is needed for the web backend to function, but I don't think any of them are in install_requires. Probably we should add pillow to the requirements because it is used in a lot of places (although technically not required if you are just using, say Qt, with just the ginga widget in a desktop app...)

@pllim
Copy link
Member Author

pllim commented Feb 20, 2021

Ah, thanks, @ejeschke ! It is fixed now.

@ejeschke
Copy link
Contributor

pycairo will give you the most capability...

pllim added a commit to pllim/astrowidgets that referenced this pull request Feb 22, 2021
@pllim
WIP: Building on astropy#126
1900081
@pllim
Copy link
Member Author

pllim commented Feb 22, 2021

Re: pycairo -- @ejeschke , for the purpose of CI, that isn't important, but maybe we can discuss this as a separate issue (#130).

@pllim pllim force-pushed the abstract-class branch 2 times, most recently from 1f3baf9 to debe04e Compare February 23, 2021 16:22
@mwcraig
Copy link
Member

mwcraig commented Feb 23, 2021

Hoping to take a look at this later today...if not, then first thing wednesday.

@pllim pllim force-pushed the abstract-class branch 2 times, most recently from a9a7c2d to a62d31f Compare February 23, 2021 16:59
@pllim
DOC: Fix inheritance diagram for Ginga API.
678a2e4 
DOC: Cannot use automodapi for Ginga API because we need to inherit docstring.

DOC: Do not display inherited members because too confusing.

[ci skip]
@pllim
Copy link
Member Author

pllim commented Feb 23, 2021

Thanks, @mwcraig ! Your feedback would be very valuable. I was just playing with finalizing the rendered API appearance and I think I finally got it working.

pllim added a commit to pllim/astrowidgets that referenced this pull request Feb 24, 2021
@pllim
WIP: Building on astropy#126
8f39bd8
This was referenced Feb 24, 2021
Closed
Closed
Closed
Base automatically changed from master to main March 9, 2021 22:47
@mwcraig mwcraig mentioned this pull request Jul 2, 2021
Draft
26 tasks
This was referenced Jul 17, 2021
Open
Draft
eteq
eteq reviewed Jul 28, 2021
View reviewed changes
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not managed a proper review of this, but given #142 I figure I should hold off anyway.

Conceptually I like this on its own, but with the caveat of all the points @mwcraig makes in #142. That is, using subclasses like this might turn off downstream users who want their astrowidgets backend to be the "real" viewer objects. It's easy to imagine a lot of dragons in managing the __init__, possible metaclasses, etc.

That said, I think basically everything in the "Understanding BaseImageWidget" section basically is still good even in the #142-style approach! That is, this captures what the user wants to see regardless of whether the backends are implemented as subclasses vs interfaces.

docs/abstract.rst

.. code-block:: python

from astrowidgets.somebackend import ImageWidget
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually I wonder if this is better presented as

Suggested change
from astrowidgets.somebackend import ImageWidget
from somevizpackage.astrowidget_backend import ImageWidget

? That is, make it clear that external packages can and should write their own backends, rather than it being an import from astrowidgets?

@eteq
Copy link
Member

eteq commented Jul 28, 2021

In some out-of-band discussion, @mwcraig, @pllim, and I concluded we want to try the Protocol approach at least to start with over the abstract/subclass approach implemented here. But the machinery from here for the documentation (i.e. https://astrowidgets--126.org.readthedocs.build/en/126/abstract.html ) and the testing machinery from here should definitely be ported to #142.

@pllim
Copy link
Member Author

pllim commented Jun 16, 2023

Superseded by #162

@pllim pllim closed this Jun 16, 2023
@pllim pllim deleted the abstract-class branch June 16, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eteq eteq eteq left review comments

@mwcraig mwcraig Awaiting requested review from mwcraig

@astrofrog astrofrog Awaiting requested review from astrofrog

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.3
4 participants
@pllim @ejeschke @mwcraig @eteq