Skip to content

New scattering factors#38

Draft
jdickerson95 wants to merge 5 commits intomainfrom
jd_new_scattering_factors
Draft

New scattering factors#38
jdickerson95 wants to merge 5 commits intomainfrom
jd_new_scattering_factors

Conversation

@jdickerson95
Copy link
Collaborator

Update to use torch-structure-manipulation to load pdb/cif model with the bonding environment.
Update to include bonded scattering factors.
Draft PR since will not merge until torch-structure-manipulation is on pypi

cropped_fft_shifted_back = torch.fft.ifftshift(cropped_fft, dim=(-3, -2))

return cropped_fft_shifted_back
if isinstance(upsampled_pixel_size, int | float | numbers.Real):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the logic behind including the number.Real class here? Was there an unhandled edge case before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch

It was a copy from torch-fourier-rescale (https://github.com/teamtomo/torch-fourier-rescale/blob/main/src/torch_fourier_rescale/fourier_rescale_3d.py)

It's definitely redundant , just numbers.Real and no int | float will catch everything

Choose a reason for hiding this comment

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

yeah can't remember what that was catching but it caught something

the fftshift sets the phase origin of the DFT to the center of the map, it is redundant for fourier rescale but necessary for fourier-slice - when I originally wrote fourier-rescale it had an output_dft kwarg and I wanted those transforms to work with fourier-slice

Comment on lines +485 to +486
# place image center at array indices [0, 0, 0] and compute centered rfft3
upsampled_volume = torch.fft.fftshift(upsampled_volume, dim=(-3, -2, -1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a requirement for the Fourier rescaling operations? Centering is not happening in Fourier space so [0, 0, 0] corresponds to the DC component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is again a copy from torch-fourier-rescale. I don't get it either.

@alisterburt, why do you do the fftshift/ifftshift in real space as well as Fourier space here? https://github.com/teamtomo/torch-fourier-rescale/blob/main/src/torch_fourier_rescale/fourier_rescale_3d.py

Is the real space shift not pointless?

Choose a reason for hiding this comment

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

the fftshift sets the phase origin of the DFT to the center of the map, it is redundant for fourier rescale but necessary for fourier-slice - when I originally wrote fourier-rescale it had an output_dft kwarg and I wanted those transforms to work with fourier-slice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah okay makes sense.

@mgiammar mgiammar added the enhancement New feature or request label Dec 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants