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

Bug fixes that produced incorrect test reports #125

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

FrankZijlstra
Copy link
Collaborator

Fixed several bugs in Sequence and test_report that result in incorrect test reports (e.g. wrong slew rates!) and incorrect k-space trajectory calculation.

Checked test reports for gre, tse, gre_radial, ute, and epi_se_rs to be identical with MATLAB.

Additionally made some minor changes that avoid some warnings (bad conditioning in polynomial fit and division by zero).

…ct test reports (e.g. wrong slew rates!) and incorrect k-space trajectory calculation.

Checked test reports for gre, tse, gre_radial, ute, and epi_se_rs to be identical with MATLAB.

Additionally made some minor changes that avoid some warnings (bad conditioning in polynomial fit and division by zero).
@btasdelen
Copy link
Collaborator

Thank you for the PR @FrankZijlstra. That is quite a substantial fix, thanks for the effort!

The patch for the test report looks straightforward, but I am not really familiar with the polynomial codebase. Maybe @sravan953 can comment on that?

An aside: I am planning to propose a PR to change the internal representation of time to an integer in units of nanoseconds rather than float in units of seconds. That should hopefully resolve the issues regarding empirical selection of eps.

@zhengliuer
Copy link

zhengliuer commented Aug 1, 2023

An aside: I am planning to propose a PR to change the internal representation of time to an integer in units of nanoseconds rather than float in units of seconds. That should hopefully resolve the issues regarding empirical selection of eps.

This is really a great idea!

@sravan953
Copy link
Collaborator

Hi all, apologies for the delay. I'll look into this in a day or two! Thank you for the PR @FrankZijlstra

@sravan953 sravan953 self-assigned this Aug 15, 2023
@sravan953 sravan953 added the bug-fix Fixes something label Aug 15, 2023
@sravan953 sravan953 merged commit 145557b into imr-framework:dev Aug 15, 2023
@FrankZijlstra FrankZijlstra deleted the fix_testreport branch August 17, 2023 09:33
@schuenke schuenke mentioned this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Fixes something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants