Skip to content

Update the output columns from fill_tics to include separation, magnitude difference, and weight#3

Merged
tylerapritchard merged 3 commits intomainfrom
fillticscolumns
Apr 2, 2025
Merged

Update the output columns from fill_tics to include separation, magnitude difference, and weight#3
tylerapritchard merged 3 commits intomainfrom
fillticscolumns

Conversation

@christinahedges
Copy link
Copy Markdown
Contributor

The fill_tics function can return the separation and magnitude difference as columns, but the function to create the target list must change those columns to ones ARK can accept. I've updated the fill_tics function and the _add_xmatch_column function to do that. I also made the xmatch column pipe delimited.

Copy link
Copy Markdown
Contributor

@tylerapritchard tylerapritchard left a comment

Choose a reason for hiding this comment

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

The functionality looks good to me and I like that we can have easier to parse feedback from fill_tics which is likely to get used internally, while maintaining the clear purpose of create_target_list, and that a few rough edges of the code were smoothed out.

Its passing tests and my quick once over on the functionality looked good.

main question - why the new target_list.csv? Was this a mistake, or is it intended to illustrate what a complete target list looks like and we should add a note to that effect in the readme somewhere? If that should this be in tutorials directory?

target_list.csv Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this intended?

@christinahedges
Copy link
Copy Markdown
Contributor Author

@tylerapritchard thank you for the review! I think the extra target list was a mistake, I've just deleted it. I have uncommented the test and put that string back in the xmatch column. I think it's useful to have it in there, especially as you are prepending a warning.

If the tests pass and you like this you can merge and release it!

@tylerapritchard tylerapritchard merged commit 77df490 into main Apr 2, 2025
6 checks passed
@tylerapritchard tylerapritchard deleted the fillticscolumns branch April 2, 2025 18:13
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.

2 participants