Skip to content

Sciquery#15

Merged
mtaghiza merged 13 commits intomasterfrom
sciquery
Jun 25, 2024
Merged

Sciquery#15
mtaghiza merged 13 commits intomasterfrom
sciquery

Conversation

@mtaghiza
Copy link
Copy Markdown
Contributor

@mtaghiza mtaghiza commented Mar 2, 2022

Adding SciQuery functionality.

Copy link
Copy Markdown
Contributor

@glemson glemson left a comment

Choose a reason for hiding this comment

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

General comment is that I think it would be good to introduce some classes, maybe one for SciQuery and one for RDBComputeDomain, against which one can call functions.
Could come with a bunch of initialization codes like SciQuery initializinng the RDBComputeDOmain-s and their dbcontexts the user has access to, the fileservice instance (generally there will be only one), setting default target volumes etc.

Copy link
Copy Markdown
Contributor

@glemson glemson left a comment

Choose a reason for hiding this comment

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

Manu, it would be good to turn this into a proper PR soon.
Leaving it as Draft so long means the final reviewers are getting a LOT of work to do.

@mtaghiza
Copy link
Copy Markdown
Contributor Author

mtaghiza commented Jun 6, 2022

@glemson Note that the amount of work is the same as before, as there are no other modules intended to be modified for this PR. Last commits are only minor updates on previous code in the same module.

@mtaghiza mtaghiza requested review from amitschang, dmedv and shandy79 June 6, 2022 15:00
@mtaghiza mtaghiza marked this pull request as ready for review June 6, 2022 15:01
Copy link
Copy Markdown

@shandy79 shandy79 left a comment

Choose a reason for hiding this comment

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

I was able to successfully run the short and long demo notebooks from Getting Started/AAS2023/SciQuery in the Astronomy image on prod.

Copy link
Copy Markdown
Contributor

@glemson glemson left a comment

Choose a reason for hiding this comment

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

go ahead.

@mtaghiza mtaghiza merged commit 87a07af into master Jun 25, 2024
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.

3 participants