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

PR 180 + pllim edits #183

Merged
merged 14 commits into from
Nov 10, 2023
Merged

PR 180 + pllim edits #183

merged 14 commits into from
Nov 10, 2023

Conversation

pllim
Copy link
Collaborator

@pllim pllim commented Oct 30, 2023

This is my edits on top of #180 by @gsanand . I was working off his branch that was already rebased on top of latest master (his original branch was outdated). And then realized too late I cannot easily open a PR to his PR in that state. And GitHub (rightfully) refused to let me force push to his fork's master branch, which he created the PR from (not recommended to create PR from fork's master but that is neither here nor there...).

Please review, @gsanand and @jryon . There are some things I am not sure about, search for "FIXME" in my diff to see them.

  • Wait for Yotam approval.
  • When squash and merge, include Yotam as co-author as well in the commit.

@@ -0,0 +1,460 @@
"""This module contains two functions that provide
means to programmatically query the `ACS/WFC
Focus-Diverse ePSF Generator <acspsf.stsci.edu>`_ API.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://acspsf.stsci.edu does not work. Is this a real URL?

Copy link
Contributor

Choose a reason for hiding this comment

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

this URL will be set to live once the module is available on acstools

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, chicken and egg then. I guess we can merge ignoring the failing link check job.

@pllim pllim added the Build wheels Build wheels for PR label Oct 31, 2023
@pllim
Copy link
Collaborator Author

pllim commented Oct 31, 2023

"Python 3.12 with remote data" (devdeps) job failure is unrelated.

Copy link
Contributor

@jryon jryon left a comment

Choose a reason for hiding this comment

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

One other general question - do we actually save the log or do the warnings/errors print to the screen?

@pllim
Copy link
Collaborator Author

pllim commented Nov 7, 2023

@gsanand , can you also please respond to this question from Jenna?

do we actually save the log or do the warnings/errors print to the screen?

@gsanand
Copy link
Contributor

gsanand commented Nov 7, 2023

@gsanand , can you also please respond to this question from Jenna?

do we actually save the log or do the warnings/errors print to the screen?

it looks like they print to the screen

@pllim pllim requested a review from yotam-stsci November 7, 2023 20:30
Copy link
Contributor

@jryon jryon left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@yotam-stsci yotam-stsci left a comment

Choose a reason for hiding this comment

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

Everything looks great. Nice work!

@pllim pllim merged commit 083c688 into spacetelescope:master Nov 10, 2023
@pllim pllim deleted the pr180-edited branch November 10, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build wheels Build wheels for PR docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants