search

astropy / pyvo

An Astropy affiliated package providing access to remote data and services of the Virtual Observatory (VO) using Python.
https://pyvo.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
74 stars 50 forks source link

Adding some documentation on pyvo.samp #558

Closed msdemlei closed 1 week ago

msdemlei commented 3 weeks ago

This has been missing so far, which is a bit of a pity given that these are fairly useful functions.

I realise we may move over ex pysamp to here at some point, and that might put the .connection() context manager discussed here into question. But then whether we have this bit of documentation or not, we cannot just break it.

ManonMarchand commented 2 weeks ago

That's nice, but maybe the file should be in a folder, either utils or its own?

Edit: And I guess there is no way we could have the example work in doctests because it requires a samp hub to be up?

msdemlei commented 2 weeks ago

On Wed, Jun 26, 2024 at 12:38:38AM -0700, Manon Marchand wrote:

That's nice, but maybe the file should be in a folder, either utils or its own?

Our systematics so far is that we reflect the organisation of the source code in docs, and samp.py, for obscure historical reasons, has been top-level.

So, as long as that's the case, I'd say the samp.rst should be top-level in docs, too. I suppose this would change when samp comes in from astropy (btw, is anyone actively pursuing it? Should we try to find someone?). If you are really concerned about a file in the top level, it could become samp/index.rst, anticipating the samp move, but I'd find that a bit odd at this point.

ManonMarchand commented 2 weeks ago

On Wed, Jun 26, 2024 at 12:38:38AM -0700, Manon Marchand wrote: That's nice, but maybe the file should be in a folder, either utils or its own? Our systematics so far is that we reflect the organisation of the source code in docs, and samp.py, for obscure historical reasons, has been top-level. So, as long as that's the case, I'd say the samp.rst should be top-level in docs, too. I suppose this would change when samp comes in from astropy (btw, is anyone actively pursuing it? Should we try to find someone?). If you are really concerned about a file in the top level, it could become samp/index.rst, anticipating the samp move, but I'd find that a bit odd at this point.

Makes sense, we can move it later when this is merged with the astropy samp module

bsipocz commented 2 weeks ago

So, as long as that's the case, I'd say the samp.rst should be top-level in docs, too. I suppose this would change when samp comes in from astropy (btw, is anyone actively pursuing it? Should we try to find someone?)

Afaik no one is actively pursuit it besides the abandoned draft PR that we have it for here. I'm not sure how much work it would be for a maintainer here to take over that PR and wrap it up (the original author has switched jobs and is no longer a contractor for astropy)

andamian commented 1 week ago

What is driving this change? Shouldn't be done after the samp port over? It's true that the port work is long overdue and there's no clear path forward but that seems to be the logical progression, unless there's an urgency to have this documentation out earlier.

msdemlei commented 1 week ago

On Tue, Jul 02, 2024 at 10:35:18AM -0700, Adrian wrote:

What is driving this change? Shouldn't be done after the samp port over? It's true that the port work is long overdue and there's no clear path forward but that seems to be the logical progression, unless there's an urgency to have this documentation out earlier.

Well, the code has been in for a long time, the API kind of makes sense, and I'm using it quite extensively in my VO course (https://codeberg.org/msdemlei/pyvo-course).

Let me quote from the Zen of Python:

Now is better than never. Although never is often better than right now.

Is there a serious reason to suspect this is a case of line two rather than line one?

andamian commented 1 week ago

On Tue, Jul 02, 2024 at 10:35:18AM -0700, Adrian wrote: What is driving this change? Shouldn't be done after the samp port over? It's true that the port work is long overdue and there's no clear path forward but that seems to be the logical progression, unless there's an urgency to have this documentation out earlier. Well, the code has been in for a long time, the API kind of makes sense, and I'm using it quite extensively in my VO course (https://codeberg.org/msdemlei/pyvo-course). Let me quote from the Zen of Python: Now is better than never. Although never is often better than right now. Is there a serious reason to suspect this is a case of line two rather than line one?

I hear you. So we have an API that's working and it's being used. We just commit to it and try to minimize changes (deprecations) when we port over SAMP. I'm all good with that.

bsipocz commented 1 week ago

Yeap, even though it wasn't officially part of the API, it's been around for a long while now with people possibly using it, so even if we technically could change it without saying anything, we shouldn't really do that. So with that, I agree that it's better to document what we have now, and then think about the porting (which we have done for a few years now, so who knows when it will actually happen).