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.

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

Change default of repeat detection tolerance in KTrajectory #554

Open
fzimmermann89 opened this issue Nov 21, 2024 · 7 comments
Open

Change default of repeat detection tolerance in KTrajectory #554

fzimmermann89 opened this issue Nov 21, 2024 · 7 comments

Comments

@fzimmermann89
Copy link
Member

fzimmermann89 commented Nov 21, 2024

repeat_detection_tolerance: float | None = 1e-8,

What does it mean to have a trajectory where all the kz's only differ by less than 1?

I think we set the default when our trajectories could also be scaled to -pi..pi
If I am not mistaken, they are now always scaled to the encoding matrix size, correct?

So we could increase this to 1.0?

@fzimmermann89
Copy link
Member Author

@ckolbPTB @schuenke Or am i wrong with the scaling?

@schuenke
Copy link
Collaborator

hmm... close to 1 probably. It might be that we expect a step of 1, but, due to rounding etc, only get 0.99. This should not be treated as "no step". So something > 0.5 and < 1.0 IMO.

@fzimmermann89
Copy link
Member Author

Is it more likely if we have a Trajektory with two values (-0.499,0.499) in kz that it really has 2 different values, or that it is a scaling issue?

@fzimmermann89
Copy link
Member Author

this is not the step, right?
isnt the logic if (max(kz)-min(kz)) < tolerance assume singleton

@ckolbPTB
Copy link
Collaborator

To me the detection tolerance should be related to numerical accuracy and rounding errors so if the step we expect is in the order of 1 then a default detection tolerance of 0.01 or 0.1 seems reasonable. By setting it too high I am afraid that we mask errors somewhere else in the code (as Patrick's example showed).

@fzimmermann89
Copy link
Member Author

There really is no reason to have anything less than 1 there.
If a dimensions does not at least have a difference of 1 between min and max, it must be singleton.
This is IMHO the better fix than conitionally rounding depending on the encoding matrix size, as done in #551.

The Fourier Op will not make any sense with steps less than 1 between min and max -- what should the result be?
Afterthing that is less than one is numerical artifact. So we can ignore it.

@fzimmermann89
Copy link
Member Author

fzimmermann89 commented Nov 21, 2024

And just removing these values would make issues even more obvious -- wrong scaling of a trajectory calculator, i.e. scaling that is incompatible with FourierOp (which assumes -enc_matrix//2 .. enc_matrix//2 scaling) would then result in a singleton Trajectory --printing it out would makeit really obvious that the trajectory is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants