Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

@garrettwrong garrettwrong commented Jul 18, 2024

Closes #1152 .

Adds pixel_size attribute to Volume and tests saving/loading default and non-default values.
Adds pixel_size attribute to Image and tests saving/loading default and non-default values.
Tests downsampling with default and non-default values.
Partially refactors Image tests to actually use fixtures (faster) and scopes them.

@garrettwrong garrettwrong added enhancement New feature or request good first issue Good for newcomers labels Jul 18, 2024
@garrettwrong garrettwrong self-assigned this Jul 18, 2024
@garrettwrong
Copy link
Collaborator Author

At this point I will move onto trying to have ImageSource serve up Images with pixel_size populated, and the associated tests for that. It wasn't really part of the original issue, but might-as-well while I'm in here.

@garrettwrong
Copy link
Collaborator Author

Some more work done here. Added pixel_size to synthetic volume generators, which should flow through from a user providing into simulation, or allow them to generate their own volumes with the attr.

After thinking about some options for dealing with filters, it seems like the only tricky case was Simulation, and its only if a user explicitly provides pixel_size and brings their own filters. I've added a simple private method to check this case and raise on mismatch. I believe all other cases should work fine with the existing filter scale approach; unless I've missed one.

Still need to add some more tests.

@garrettwrong
Copy link
Collaborator Author

Confirmed the pixel_size is being picked up in ChimeraX when saved from ASPIRE.

Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

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

This looks great. Just a few things.

@garrettwrong garrettwrong requested a review from j-c-c August 1, 2024 16:13
j-c-c
j-c-c previously approved these changes Aug 1, 2024
Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

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

Looks good!

@garrettwrong garrettwrong marked this pull request as ready for review August 2, 2024 11:53
@garrettwrong garrettwrong requested a review from janden as a code owner August 2, 2024 11:53
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for tracking down the pixel_sizes wherever they've managed to hide themselves. Just a few questions here.

@garrettwrong
Copy link
Collaborator Author

Rebased to resolve minor downsample merge ocnflicts.

@garrettwrong garrettwrong merged commit b843a31 into develop Aug 27, 2024
@garrettwrong garrettwrong deleted the pxsz branch August 27, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants