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

Threshold for pixel spacing #95

Merged
merged 0 commits into from
Jun 11, 2023
Merged

Threshold for pixel spacing #95

merged 0 commits into from
Jun 11, 2023

Conversation

eric-unc
Copy link
Collaborator

Completion of #75

@eric-unc eric-unc requested a review from jesse-wei June 10, 2023 04:19
@eric-unc
Copy link
Collaborator Author

Feel free to recommend rephrasing of anything and refactoring (or just push to my branch). I don't remember how to resize images using whatever tool it is so I only tested that it didn't break current things. @jesse-wei

jesse-wei
jesse-wei reviewed Jun 10, 2023
View reviewed changes
NeuroRuler/utils/img_helpers.py Outdated
Comment on lines 179 to 182
is_center_eq = props1[0] == props2[0]
is_size_eq = props1[1] == props2[1]
spacing1 = props1[2]
spacing2 = props2[2]
Copy link
Collaborator

@jesse-wei jesse-wei Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I originally chose to use a tuple for properties, but indexing by number only is bad design. We should instead use named tuples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do this later.

jesse-wei
jesse-wei reviewed Jun 10, 2023
View reviewed changes
gui_config.json Outdated Show resolved Hide resolved
@eric-unc eric-unc requested a review from jesse-wei June 11, 2023 02:59
@eric-unc
Copy link
Collaborator Author

Feel free to re-review. Also would be good if it's actually tested lol, preferably automatically.

@jesse-wei
Copy link
Collaborator

Feel free to re-review. Also would be good if it's actually tested lol, preferably automatically.

I also don't know how to modify pixel spacing value. We will shrimply test using our brains.

jesse-wei
jesse-wei approved these changes Jun 11, 2023
View reviewed changes
Copy link
Collaborator

@jesse-wei jesse-wei left a comment

Choose a reason for hiding this comment

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

LGTM

mlouielu
mlouielu reviewed Jun 11, 2023
View reviewed changes
Copy link
Contributor

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

Could improve how the type hint works in NamedTuple

NeuroRuler/utils/img_helpers.py Outdated Show resolved Hide resolved
@mlouielu
Copy link
Contributor

I don't think CI has mypy check at this moment, you can rebase this branch to main and run pre-commit run --all-files to check with mypy.

@jesse-wei jesse-wei merged commit 5b402b6 into main Jun 11, 2023
@eric-unc eric-unc deleted the eric/threshhold branch June 11, 2023 16:18
@jesse-wei jesse-wei mentioned this pull request Jun 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jesse-wei jesse-wei jesse-wei approved these changes

@mlouielu mlouielu mlouielu left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@eric-unc @jesse-wei @mlouielu