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

T2* mapping model #445

Open
wants to merge 16 commits into
base: usrjson
Choose a base branch
from
Open

T2* mapping model #445

wants to merge 16 commits into from

Conversation

jvelazquez-reyes
Copy link
Collaborator

@jvelazquez-reyes jvelazquez-reyes commented Jun 24, 2021

Purpose

T2* mapping model working with default parameters

Open Questions and Pre-Merge TODOs

  • Use github checklists. When solved, check the box and explain the answer.

  • Review that changed source files/lines are related to the pull request/issue
    If any files/commits were accidentally included, cherry-pick them into another branch.

  • Review that changed source files/lines were not accidentally deleted
    Fix appropriately if so.

  • Test new features or bug fix
    If not implemented/resolved adequately, solve it or inform the developer by requesting changes in your review.
    Preferably, set breakpoints in the locations that the code was changed and follow allong line by line to see if the code behaves as intended.

Manual GUI tests (general)
  • Does the qMRLab GUI open?
  • Can you change models?
  • Can you load a data folder for a model?
  • Can you view data?
  • Can you zoom in the image?
  • Can you pan out of the image?
  • Can you view the histogram of the data?
  • Can you change the color map?
  • Can you fit dataset (Fit data)?
  • Can you save/load the results?
  • Can you open the options panel?
  • Can you change option parameters?
  • Can you save/load option paramters?
  • Can you select a voxel?
  • Can you fit the data of that voxel ("View data fit")?
  • Can you simulate and fit a voxel ("Single Voxel Curve")?
  • Can you run a Sensitivity Analysis?
  • Can you simulate a Multi Voxel Distribution?

@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #445 (f24407b) into usrjson (e2d2140) will decrease coverage by 1.09%.
The diff coverage is 47.04%.

Impacted file tree graph

@@             Coverage Diff             @@
##           usrjson     #445      +/-   ##
===========================================
- Coverage    47.40%   46.30%   -1.10%     
===========================================
  Files          260      282      +22     
  Lines         8647    10370    +1723     
===========================================
+ Hits          4099     4802     +703     
- Misses        4548     5568    +1020     
Flag Coverage Δ
matlab 45.48% <46.55%> (-1.14%) ⬇️
octave 38.37% <46.26%> (+1.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Common/AbstractModel.m 75.00% <ø> (+8.75%) ⬆️
src/Common/FitResultsSave_BIDS.m 0.00% <0.00%> (ø)
src/Common/qMRFit_BIDS.m 0.00% <0.00%> (ø)
src/Common/qMRLab_static_Models.m 0.00% <0.00%> (ø)
src/Common/units/validatePanelUnits.m 0.00% <0.00%> (ø)
...els_Functions/MP2RAGE/func/T1B1correctpackageTFL.m 84.74% <ø> (ø)
src/Models_Functions/T2star/TA_rget.m 0.00% <0.00%> (ø)
src/Models_Functions/T2star/conv3.m 0.00% <0.00%> (ø)
src/Models_Functions/T2star/stat_nlleasqr.m 0.00% <0.00%> (ø)
.../Models_Functions/T2star/t2star_computeGradientZ.m 0.00% <0.00%> (ø)
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2d2140...f24407b. Read the comment docs.

@agahkarakuzu
Copy link
Member

@jvelazquez-reyes for tests we may consider a really small dataset for this model.

@jvelazquez-reyes
Copy link
Collaborator Author

Is there any available on OSF?

@agahkarakuzu
Copy link
Member

Not yet, but if you can crop the images to a smaller extent, we can add it.

@agahkarakuzu
Copy link
Member

Great! Let's make sure that the test actually runs for this model. Then we'll merge usejson first to master.

@agahkarakuzu
Copy link
Member

agahkarakuzu commented Oct 28, 2021

@jvelazquez-reyes see this error.

The test is failing because the filenames you provide for the example dataset do not match those in InputNames of the model.

Current file names:
magnitude.nii.gz phase.nii.gz

Required file names:
DATAmag.nii.gz DATAphase.nii.gz

That's how genBatch infers which file to load to which field.

Can you also update the mono_t2star header comment to make separate entries for them?

@jvelazquez-reyes
Copy link
Collaborator Author

I see, thanks!

What names would you prefer to keep? magnitude.nii.gz or DATAmag.nii.gz?

@agahkarakuzu
Copy link
Member

It is not really up to my preference :) Please see the wiki docs:

https://github.com/qMRLab/qMRLab/wiki/Guideline:-Uploading-sample-data

@agahkarakuzu
Copy link
Member

Cool, I like Magn and Phase better and now they match OSF vs Model 👍

@jvelazquez-reyes jvelazquez-reyes changed the base branch from master to usrjson July 13, 2022 21:06
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.

None yet

3 participants