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.

Sign up for GitHub

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

Jump to bottom

Align multiple visits notebook, new pull request #300

Merged
merged 20 commits into from
Sep 19, 2024
Merged

Align multiple visits notebook, new pull request #300

merged 20 commits into from
Sep 19, 2024

Conversation

gsanand
Copy link
Contributor

@gsanand gsanand commented Jul 31, 2024

new pull request for align_multiple_visits (it was difficult for me to keep track with the previous pull request and the many changes).

@haticekaratay @mackjenn I believe this is working now, other than the bandit error.

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@gsanand
Copy link
Contributor Author

gsanand commented Jul 31, 2024 •
edited
Loading

okay well I take that back, apparently there is some issue with the requirements file that was not there in the previous pull request.

edit: it is using the top level requirements file in the main directory which I don't understand why....

@haticekaratay
Copy link
Collaborator

okay well I take that back, apparently there is some issue with the requirements file that was not there in the previous pull request.

edit: it is using the top level requirements file in the main directory which I don't understand why....

@gsanand, you have 2 notebooks attached. Can you remove the one that's not in the
notebooks/DrizzlePac/align_multiple_visits/ directory tree?

@gsanand
Copy link
Contributor Author

gsanand commented Aug 1, 2024

okay well I take that back, apparently there is some issue with the requirements file that was not there in the previous pull request.
edit: it is using the top level requirements file in the main directory which I don't understand why....

@gsanand, you have 2 notebooks attached. Can you remove the one that's not in the notebooks/DrizzlePac/align_multiple_visits/ directory tree?

thanks, not sure how that happened. fixed now!

@haticekaratay haticekaratay self-requested a review August 28, 2024 19:05
haticekaratay
haticekaratay reviewed Sep 3, 2024
View reviewed changes
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
notebooks/DrizzlePac/align_multiple_visits/align_multiple_visits.ipynb Show resolved Hide resolved
@haticekaratay
Copy link
Collaborator

haticekaratay commented Sep 18, 2024 •
edited
Loading

Hey @gsanand , I fixed the image-related error by adding the images with the required path to the directory. However, I'm facing a new issue on both local and CI environments. This is causing a false positive. Can you please investigate the most recent failure related to tweakreg? You can check the CI logs for the specific execution step to see the error. Also, no need to worry about the bandit issue, I'll take care of that.

haticekaratay
haticekaratay approved these changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@haticekaratay haticekaratay left a comment

Choose a reason for hiding this comment

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

Thank you for implementing the suggestion and confirming the outputs. Everything looks good now.

@haticekaratay haticekaratay merged commit 8489bc6 into spacetelescope:main Sep 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haticekaratay haticekaratay haticekaratay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gsanand @haticekaratay