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

DCE-dro #39

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

DCE-dro #39

wants to merge 12 commits into from

Conversation

MohamedNasser8
Copy link
Collaborator

@MohamedNasser8 MohamedNasser8 commented Jul 18, 2024

PR: Updates to DRO Code

I have made some updates to the DRO code, based on the existing code from TF6.

Changes Made:

  1. Removed Extra Code for Reading Functions:

    • The original code included separate functions for reading slices, 4D data, and DICOM files. I streamlined this by removing the extra reading functions.
    • I replaced these with a single function read_dicoms_as_4d_signal which takes a path and returns a 4D MR dataset.
  2. Added Animation Functionality:

    • The existing animation function wasn't working properly, so I updated it.
    • The new function allows for displaying animations of different slices or the same slice animated over time.
  3. Removed Directory Creation and Data Saving Code:

    • I removed the code responsible for creating directories and saving data for now, to simplify the codebase.
  4. Additions

    • I implemented Tofts and Extended Tofts models using a parallel approach as their fitting function is time-consuming.
    • Used OSIPI methods to calculate the concentration of the tissue after fitting.

@MohamedNasser8 MohamedNasser8 changed the title Dce dro DCE-dro Jul 18, 2024
@MohamedNasser8 MohamedNasser8 requested a review from ltorres6 July 18, 2024 23:20
@ltorres6 ltorres6 changed the base branch from main to docs August 5, 2024 14:56
@ltorres6 ltorres6 changed the base branch from docs to main August 8, 2024 14:56
Copy link
Collaborator

@LucyKershaw LucyKershaw left a comment

Choose a reason for hiding this comment

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

I think the issue with this code is that it still rewrites some of the existing osipi functions instead of using them directly, e.g. osipi.S_to_C_via_R1_SPGR

There is a lot of good work here, I'm keen to see the documentation

@MohamedNasser8
Copy link
Collaborator Author

@LucyKershaw Yeah, initially I was copying the code as is without modifying the functions, but I'll update it to rely on the osipi functions. There were a lot of questions and some lines that were unclear to me, which is why I didn't change them earlier.

@ltorres6
Copy link
Collaborator

ltorres6 commented Sep 4, 2024

@MohamedNasser8 Can you update this branch to use existing functions to remove redundancy?

Comment on lines 145 to 221
# trans_K1 = mf_K1.copy()
# trans_k2 = mf_k2.copy()
# trans_Vp = mf_Vp.copy()
#
# vmax_K1, vmax_k2, vmax_Vp = 1, 1, 0.2
# vmin_K1, vmin_k2, vmin_Vp = 0.2, 0.2, 0.01
# lb_K1, lb_k2, lb_Vp = 0.54, 0.52, 0.49
# ub_K1, ub_k2, ub_Vp = 1.52, 1.5, 1.43
# lim_K1, lim_k2, lim_Vp = vmax_K1 + 0.5, vmax_k2 + 0.1, vmax_Vp + 0.5
# ub_lim = 1.01
#
# trans_K1[trans_K1 <= vmin_K1] = trans_K1[trans_K1 <= vmin_K1] * lb_K1
# trans_K1[trans_K1 >= lim_K1] = trans_K1[trans_K1 >= lim_K1] * ub_lim
# trans_K1[(trans_K1 >= vmax_K1) & (trans_K1 < lim_K1)] = (
# trans_K1[(trans_K1 >= vmax_K1) & (trans_K1 < lim_K1)] * ub_K1
# )
# trans_K1[(trans_K1 > vmin_K1) & (trans_K1 < vmax_K1)] = trans_K1[
# (trans_K1 > vmin_K1) & (trans_K1 < vmax_K1)
# ] * (
# lb_K1
# + (
# (
# (trans_K1[(trans_K1 > vmin_K1) & (trans_K1 < vmax_K1)] - vmin_K1)
# / (vmax_K1 - vmin_K1)
# )
# * (ub_K1 - lb_K1)
# )
# )
#
# trans_k2[trans_k2 <= vmin_k2] = trans_k2[trans_k2 <= vmin_k2] * lb_k2
# trans_k2[trans_k2 >= lim_k2] = trans_k2[trans_k2 >= lim_k2] * ub_lim
# trans_k2[(trans_k2 >= vmax_k2) & (trans_k2 < lim_k2)] = (
# trans_k2[(trans_k2 >= vmax_k2) & (trans_k2 < lim_k2)] * ub_k2
# )
# trans_k2[(trans_k2 > vmin_k2) & (trans_k2 < vmax_k2)] = trans_k2[
# (trans_k2 > vmin_k2) & (trans_k2 < vmax_k2)
# ] * (
# lb_k2
# + (
# (
# (trans_k2[(trans_k2 > vmin_k2) & (trans_k2 < vmax_k2)] - vmin_k2)
# / (vmax_k2 - vmin_k2)
# )
# * (ub_k2 - lb_k2)
# )
# )
#
# trans_Vp[trans_Vp <= vmin_Vp] = trans_Vp[trans_Vp <= vmin_Vp] * lb_Vp
# trans_Vp[(trans_Vp >= lim_Vp)] = trans_Vp[trans_Vp >= lim_Vp] * ub_lim
# trans_Vp[(trans_Vp >= vmax_Vp) & (trans_Vp < lim_Vp)] = (
# trans_Vp[(trans_Vp >= vmax_Vp) & (trans_Vp < lim_Vp)] * ub_Vp
# )
# trans_Vp[(trans_Vp > vmin_Vp) & (trans_Vp < vmax_Vp)] = trans_Vp[
# (trans_Vp > vmin_Vp) & (trans_Vp < vmax_Vp)
# ] * (
# lb_Vp
# + (
# (
# (trans_Vp[(trans_Vp > vmin_Vp) & (trans_Vp < vmax_Vp)] - vmin_Vp)
# / (vmax_Vp - vmin_Vp)
# )
# * (ub_Vp - lb_Vp)
# )
# )
#
# trans_Vp[trans_Vp > 1] = 1
#
# Ctiss_tr = forward_extended_tofts(trans_K1, trans_k2, trans_Vp, Ea, t)
#
# R1_tr, R2st_tr = calcR1_R2(R10, R20st_value, r1, r2st, Ctiss_tr)
# dro_S_tr, M_tr = Conc2Sig_Linear(Tr, Te, fa, R1_tr, R2st_tr, signal[:, :, :8, :], 1, M)
#
# dro_Snoise_tr = addnoise(1, stdS, dro_S_tr, Nt=data_shape[-1])
#
# animate_mri(signal, mode="time", slice_index=7, time_index=5)
# animate_mri(dro_Snoise_tr, mode="time", slice_index=7, time_index=5)
# animate_mri(dro_Snoise, mode="time", slice_index=7, time_index=5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove.

@ltorres6
Copy link
Collaborator

ltorres6 commented Sep 4, 2024

In addition to what Lucy mentioned above:

  • Please use snake_case for each of the functions and CamelCase for the classes.
  • These functions need unit tests. We will discuss code coverage in our next meeting. This will be an additional hook, and we will set code coverage to 95% of code.
  • The display functions should go under a utilities module. Now is a good time to create the osipi.utils module.

- add imports as osipi dro
- refactor function name to be snake case
- added save function in utils
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