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.

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

DM-35962: Remove examples/ from all packages #15

Merged
merged 1 commit into from Apr 4, 2023
Merged

Conversation

parejkoj
Copy link
Contributor

@parejkoj parejkoj commented Jan 3, 2023

No description provided.

@parejkoj parejkoj requested a review from timj January 3, 2023 21:37
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

This being a notebook makes me wonder if the content of the notebook covers items that are missing from the documentation.

@parejkoj
Copy link
Contributor Author

parejkoj commented Jan 5, 2023

The notebook here doesn't have any narrative structure, so it's not clear what it's supposed to be demonstrating.

This package is missing from the pipeline docs, and doesn't have a correctly-configured doc/. It's not even clear the package works, since the single test file does nothing.

@timj
Copy link
Member

timj commented Jan 5, 2023

Ok. The lack of tests in the display packages has always been a problem (it seems there should be a set of tests in afw that the display packages could reuse).

@gpdf
Copy link

gpdf commented Jan 5, 2023

We also clearly have issues in the ability of users to treat afw.display as clearly a backend-agnostic base. I have recently seen examples where people have completely different call sequences to afw.display depending on which back end they are using.

I think we need a sprint on improving the situation, which could include releasing some more solid tutorial code in a CI-able place. I've been discussing this with @frossie .

@gpdf
Copy link

gpdf commented Jan 5, 2023

(no objections to removing this particular notebook, for now)

@RobertLuptonTheGood
Copy link
Member

RobertLuptonTheGood commented Jan 5, 2023 via email

@gpdf
Copy link

gpdf commented Jan 5, 2023

display_astrowidgets, I think you mean?

@gpdf
Copy link

gpdf commented Jan 5, 2023

I'm mostly thinking of things I've seen in people's personal code, and I'm not trying to embarrass anyone. It looked to me like the issue was that laying out a multi-image display grid can't be done in a backend-independent way. I haven't had the time to dig into it - I don't know if it's a weakness in the common API, or a (fixable) difference in how the backends respond to it, or what.

It's also the case that displaying a catalog over an image from afw.display is being done in a way that makes it impossible to take advantage of a lot of the power of Firefly for working with overlays. We need to come back to that point - we stalled out on that, collectively, when it first came up.

We haven't seriously looked at this whole system since ~2018, so I think it's time for a sprint on this.

@parejkoj
Copy link
Contributor Author

parejkoj commented Apr 4, 2023

Finally getting back to this: the discussion above was useful, but there were no objections to removing this notebook, so I am going to merge this ticket.

@parejkoj parejkoj merged commit 1ea34f9 into main Apr 4, 2023
@parejkoj parejkoj deleted the tickets/DM-35962 branch April 4, 2023 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants