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

WIP: add basic SOFIA query tool #2711

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

keflavich
Copy link
Contributor

Solves #2232, but very much a WIP. I've gotten the server to return data, finally, but I haven't parsed it yet.

@keflavich keflavich marked this pull request as draft April 20, 2023 01:54
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #2711 (3c24761) into main (1ba336e) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 3c24761 differs from pull request most recent head 1f4a8cb. Consider uploading reports for the commit 1f4a8cb to get more accurate results

@@           Coverage Diff           @@
##             main    #2711   +/-   ##
=======================================
  Coverage   65.74%   65.75%           
=======================================
  Files         233      233           
  Lines       17844    17846    +2     
=======================================
+ Hits        11732    11734    +2     
  Misses       6112     6112           

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

@keflavich - We've briefly discussed this, and would prefer to have one layer flatter namespace for the different irsa services, e.g. have astroquery.ipac.irsa.Irsa to handle the generic services based on tap, sia, etc, but have the one-off ones in this namespace, too, e.g. astroquery.ipac.irsa.Most and astroquery.ipac.irsa.Sofia I know that ibe and sha currently is one layer down, but those are legacy services that got moved unto astroquery.ipac.irsa from the main namespace.

@keflavich
Copy link
Contributor Author

Right, I forgot about that. Moved.

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

Oh, what I really meant is to not have a sofia submodule, but have the class directly in the irsa namespace (e.g. preferably in a sofia.py file, instead of core.py, etc.), similarly how mast has multiple classes in the mast namespace.

The only possible complication I see is the shared default config.

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

(also, something went wrong with the move, and now you've deleted all the files rather than moved them)

@keflavich
Copy link
Contributor Author

🤦 added it back.

But it might be a good idea to keep this one separate within this space because the implementation is so different? I'm ok either way; I'm not sure when I'm going to finish this though.

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

My thinking is these are all IRSA services, and the added namespace doesn't really adds much. I would not suggest to add support for any of the mission specific services in the Irsa() class, but to not have an additional layer such as astroquery.ipac.irsa.sofia.Sofia(), but expose it directly in astroquery.ipac.irsa.Sofia()

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

As for docs, having a separate sofia.rst would be nice, and I'll suggest chopping up mast.rst, too.

@keflavich
Copy link
Contributor Author

Ah, ok, I can lower the namespace a level. I think it's helpful to have sofia.py though.

and yes, sofia.rst for docs sounds good

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

I think it's helpful to have sofia.py though.

Yeap, each service class should have its own file. I'll do an audit on sha and ibe, too, if they will still be needed after we switch to the VO backends, I think they should also be exposed in the irsa namespace.

@bsipocz
Copy link
Member

bsipocz commented Dec 7, 2023

@keflavich - FYI: SOFIA datasets are getting moved to be served by the VO backends, so they will be available using the generic irsa module (at least the images will be available, we don't yet have an SSA layer, but if needed that can be done and is preferred as opposed to a custom sofia only tool). I would suggest this module won't be necessary and the PR should eventually be closed without merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants