-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add a ProjectionModel
specification
#20
Add a ProjectionModel
specification
#20
Conversation
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.
Nice, stoked to see this stuff coming in! A few comments here and there :)
README.md
Outdated
| `rotation_x` | ROTATION_X | tomogram rotation around x-axis | | ||
| `rotation_y` | ROTATION_Y | tomogram rotation around y-axis | | ||
| `rotation_z` | ROTATION_Z | tomogram rotation around z-axis | | ||
| `dx` | SHIFT_X | particle shift in x-dimension | | ||
| `dy` | SHIFT_Y | particle shift in y-dimension | |
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'm a bit confused by these headers. This is about tilt images right? so not about tomograms or particles?
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.
sorry, forgot to update the shifts labels in the table copied from poses!
The way rotations are parametrised for tilt-series alignments in all softwares (AFAIK) is in terms of 'rotations of the specimen' i.e. in the microscope reference frame
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.
ok, makes sense!
```python | ||
from cryotypes.projectionmodel import projection_model_to_projection_matrices | ||
|
||
projection_matrices = projection_model_to_projection_matrices( | ||
df=projection_model, # ProjectionModel dataframe | ||
tilt_image_center=(1919, 1355), # tilt-image rotation center (xy) | ||
tomogram_dimensions=(3838, 3710, 2000) # dimensions of tomogram (xyz) | ||
) | ||
``` |
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 we should provide the same for particles (if one wants to work with affines rather than pos + rot).
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.
possible but not sure of the benefit there, will open an issue to discuss something re this though
| `pixel_spacing` | PIXEL_SPACING | isotropic pixel/voxel spacing for shifts | | ||
| `source` | SOURCE | reference to the file from which data came | | ||
|
||
A utility function is also provided for generating projection matrices from these data. |
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.
What transformation are these proj matrices describing exactly?
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.
position in tomogram -> position in tilt image, one matrix per tilt image
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.
As in, if you apply this to the real object and project along z, you get the tilt image? I would add a small note for this for clarity!
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.
yes, if by z you mean the beam axis :)
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.
note added
ROTATION_X = "rotation_x" | ||
ROTATION_Y = "rotation_y" | ||
ROTATION_Z = "rotation_z" | ||
ROTATION = [ROTATION_X, ROTATION_Y, ROTATION_Z] |
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.
Should we maybe for consistency use actual Rotation
objects internally?
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.
agree we want consistency but don't think a Rotation is simpler for this use case where a primary use case could be checking how much things have shifted from expected stage tilt angles -> I don't want to force understanding rotation decomposition on someone in that situation
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.
We can always provide utility functions... I mean, this still requires all the convention stuff from euler angles, so at least the Rotation moves the burden to the creation of the object, instead of both creation and utilisation. I just think consistency here would be nice to keep; and like with poses, it's just more convenient to work with single values rather than multiple things.
Either way, we should at least clarify in which order rotations happen, if they are intrinsic/extrinsicm, yada yada.
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.
note added explaining rotations!
Codecov Report
@@ Coverage Diff @@
## main #20 +/- ##
===========================================
- Coverage 75.65% 58.89% -16.77%
===========================================
Files 11 16 +5
Lines 267 343 +76
===========================================
Hits 202 202
- Misses 65 141 +76
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
thanks for the review, notes added! rotation object discussion can continue in #21 - will merge here |
No description provided.