Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[feat][METEOR-1128] TDCT Odemis Wrapper #2999
[feat][METEOR-1128] TDCT Odemis Wrapper #2999
Changes from all commits
6f18d8d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this argument is only present in case of failure, in which case it will be returned without manipulation. To me it makes more sense to not make it an arg/return but just throw the error (or return None, since no optimal z was found) and handle the failure case outside of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point but disagree, almost always the handling will be to use the initial value. If it becomes an issue it can be changed later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is confusing that some coordinates are saved in pixels and some in m, while all of them are called as coords? I am not sure why poi_coord is in m and not in pixels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the final output for the milling patterns, which have coordinates in metres. They are all coordinates, not sure if you have a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if most of the coordinates are in pixels, by default the coords can represent pixels and for m it can be represented by pos , while the function name can explicitly tell i.e. in meters ? For e.g. get_poi_pos() or get_poi_coords_in_meters().
With that I would rename the function description and definitions to make the difference clear.