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

Testing/template data #8

Merged
merged 9 commits into from
Sep 28, 2023
Merged

Testing/template data #8

merged 9 commits into from
Sep 28, 2023

Conversation

etpeterson
Copy link
Contributor

This should be reviewed after Oliver's added_phantom

@@ -2,6 +2,8 @@
import numpy.testing as npt
import pytest
import torch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need torch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'll remove it. I think I added it because it seemed like it was being used by other people and I wanted to add it as an option but ended up not doing that.

print(f'{name} {data}')
signal = np.asarray(data['data'])
signal /= signal[0]
if data['f'] == 1.0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why this is here. Would you not also need a if f==0 statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's really just because of my linear approximation fitting. I wouldn't expect real fitting algorithms to need it.

It's because it tries to fit non-flow data first and if it's all flow, it assumes that's non-flow and returns incorrect values. That's also why f==0 is not needed. In that case it already greedily fit the non-flow and there's nothing left for flow to fit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could not find how this Jason file was made and what is in it. Was that done in previous commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is created around here. https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/pull/8/files#diff-4407951042bb3aaf1228971e5efe98aaf394e4dcd2fd73ee6de10776e4fdd59bR397

I think once I update mine to recognize that yours was merged the changes will be more clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why XCAT_MAT_RESP is added to your code? Or is it because it is still a remnant from my merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's from yours because I based mine off of yours. I was expecting those to disappear when yours was merged, but it didn't. I'll try rebasing or merging main to mine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, looks like the merge cleaned it up

@etpeterson
Copy link
Contributor Author

This should be reviewed after Oliver's added_phantom

This is done so now the diff is clean.

@DKuppens
Copy link
Contributor

@etpeterson is this ready to merge?

@etpeterson
Copy link
Contributor Author

@etpeterson is this ready to merge?

Yes!

@DKuppens DKuppens merged commit be798d3 into main Sep 28, 2023
IvanARashid added a commit that referenced this pull request Sep 29, 2023
commit 1b20d78
Author: Oscar Jalnefjord <[email protected]>
Date:   Thu Sep 28 19:23:54 2023 +0200

    MATLAB code contribution

commit be798d3
Merge: ff3fcbe 70d06a0
Author: Daan Kuppens <[email protected]>
Date:   Thu Sep 28 17:06:29 2023 +0200

    Merge pull request #8 from OSIPI/testing/template_data

    Testing/template data

commit ff3fcbe
Merge: 99ca3b6 ab5ec3a
Author: Daan Kuppens <[email protected]>
Date:   Thu Sep 28 11:39:43 2023 +0200

    Merge pull request #11 from OSIPI/data_addition

    data addition and configuration of LFS - approved by Eric

commit ab5ec3a
Author: Daan <[email protected]>
Date:   Fri Sep 15 12:23:05 2023 +0200

    fix typo

commit 7bd1692
Author: Daan <[email protected]>
Date:   Fri Sep 15 12:22:00 2023 +0200

    update gitignore and gitattributes

commit 73c72fc
Author: Daan <[email protected]>
Date:   Fri Sep 15 12:02:54 2023 +0200

    data addition and configuration of LFS
@etpeterson etpeterson deleted the testing/template_data branch November 6, 2023 23:10
etpeterson pushed a commit to etpeterson/TF2.4_IVIM-MRI_CodeCollection that referenced this pull request Jul 19, 2024
etpeterson pushed a commit to etpeterson/TF2.4_IVIM-MRI_CodeCollection that referenced this pull request Jul 19, 2024
commit e030c14
Author: Oscar Jalnefjord <[email protected]>
Date:   Thu Sep 28 19:23:54 2023 +0200

    MATLAB code contribution

commit a36245f
Merge: ff3fcbe 70d06a0
Author: Daan Kuppens <[email protected]>
Date:   Thu Sep 28 17:06:29 2023 +0200

    Merge pull request OSIPI#8 from OSIPI/testing/template_data

    Testing/template data

commit ebdb475
Merge: 99ca3b6 ab5ec3a
Author: Daan Kuppens <[email protected]>
Date:   Thu Sep 28 11:39:43 2023 +0200

    Merge pull request OSIPI#11 from OSIPI/data_addition

    data addition and configuration of LFS - approved by Eric

commit 7562bfe
Author: Daan <[email protected]>
Date:   Fri Sep 15 12:23:05 2023 +0200

    fix typo

commit 7074e54
Author: Daan <[email protected]>
Date:   Fri Sep 15 12:22:00 2023 +0200

    update gitignore and gitattributes

commit 666b158
Author: Daan <[email protected]>
Date:   Fri Sep 15 12:02:54 2023 +0200

    data addition and configuration of LFS
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

Successfully merging this pull request may close these issues.

3 participants