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

incorporate findsat_mrt into acstools #176

Merged
merged 121 commits into from
May 8, 2023
Merged

Conversation

dvstark
Copy link
Contributor

@dvstark dvstark commented Jan 6, 2023

Description

This pull request is incorporate newcode (findsat_mrt) into acstools. This code, described in ISR 2022-08, provides a new framework for identifying and masking satellite trails in ACS/WFC data. It should not impact any existing acstools programs.

@github-actions github-actions bot added the docs label Jan 6, 2023
@dvstark dvstark assigned pllim, jryon and rjavila and unassigned pllim, jryon and rjavila Jan 9, 2023
@dvstark dvstark requested review from pllim, jryon and rjavila January 9, 2023 18:38
@dvstark
Copy link
Contributor Author

dvstark commented Jan 9, 2023

I'm aware of the odd line breaks with ^M at the end, and am trying to fix it

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I have a bunch of proposed changes over at dvstark#1 . These include API changes and bug fixes as we discussed on Slack. I also applied changes from my earlier rounds of reviews. Reading the diff might drive you crazy, so it might be better for you to view the before/after side by side.

Since these include real bug fixes, if you run the unit test that you wrote with my changes, the result now disagrees with the "truth" you provided here. I cannot tell if that is okay or not, so please look at it carefully.

We can discuss via meeting the week after next if you are unable to review it on your own. Thank you for your continued patience!

@pllim
Copy link
Collaborator

pllim commented May 5, 2023

A minor PEP 8 thingy to fix.

And also the link appears broken: https://www.clear.rice.edu/elec431/projects96/DSP/bpanalysis.html

@dvstark
Copy link
Contributor Author

dvstark commented May 5, 2023

I'm suspecting that broken link was temporary; it seems to be working now. I'll fix the pep8 stuff and push again

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Just some minor comments. I'll approve when they are addressed.

By the way, CI was green after I reran the failed jobs, so the linkcheck and socket failures were indeed transient.

@dvstark
Copy link
Contributor Author

dvstark commented May 7, 2023

All your suggested changes made sense so I committed them all (and sorry if this led to various email notifications from github after every commit)

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks! @jryon , do you want to re-review since you requested changes?

If you happen to merge first, please use the Squash and Merge button.

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.

I'm happy with it now that @pllim and @dvstark are!

@pllim pllim merged commit 70f0e2f into spacetelescope:master May 8, 2023
@pllim
Copy link
Collaborator

pllim commented May 8, 2023

Merged. Congrats and thanks for your patience!

@jryon , you are doing releases now, right?

@jryon
Copy link
Contributor

jryon commented May 8, 2023

Yep, added to my to-do list.

@dvstark
Copy link
Contributor Author

dvstark commented May 8, 2023

Thanks both!

@dvstark dvstark deleted the findsat_mrt branch February 28, 2024 15:39
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.

4 participants