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

DM-38744: Add auto option for how main star centroid is picked #30

Merged
merged 3 commits into from Apr 18, 2023

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

Copy link

@czwa czwa left a comment

Choose a reason for hiding this comment

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

Minor docstring suggestion.


Parameters
----------
inputCentroid : `???`
Copy link

Choose a reason for hiding this comment

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

dict with no further information is better than nothing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, yeah, that wasn't intentional, sorry 😄

self.log.info("Auto centroid is using FIT in Spectractor to get the target centroid")
return 'fit' # this means exact

# this is just renaming the config parameter because guess sounds like
Copy link

Choose a reason for hiding this comment

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

This is confusing, but from this comment and those above, it seems that is just how it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's very confusing, hence choosing to rename it for us. I can negotiate renaming with Jérémy, but given we rename basically all the spectractor parameters into task config options, changing some of the values too doesn't feel so bad. I'll have a chat with him and if he agrees we can take it out in future.

@mfisherlevine mfisherlevine merged commit 39be758 into main Apr 18, 2023
1 check passed
@mfisherlevine mfisherlevine deleted the tickets/DM-38744 branch April 18, 2023 23:33
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.

None yet

2 participants