search

yunshiuan / label4MRI

Label the brain MNI coordinate by AAL/BA system
GNU General Public License v3.0
55 stars 19 forks source link

Make data internal #14

Closed psychelzh closed 10 months ago

psychelzh commented 11 months ago

This package includes external data in "data" folder, but seemingly it is only used internally. So if I use a call with namespace without attaching the package, the data cannot be found. 

label4MRI::list_brain_regions()
#> Error in label4MRI::list_brain_regions(): 找不到对象'label4mri_metadata'

Created on 2023-08-11 with reprex v2.0.2

psychelzh commented 11 months ago

I have made a PR to fix this, and after that, the call with namespace without attaching would work:

label4MRI::list_brain_regions()
#> $aal
#>   [1] "Precentral_L"         "Precentral_R"         "Frontal_Sup_L"       
#>   [4] "Frontal_Sup_R"        "Frontal_Sup_Orb_L"    "Frontal_Sup_Orb_R"   
#>   [7] "Frontal_Mid_L"        "Frontal_Mid_R"        "Frontal_Mid_Orb_L"   
#>  [10] "Frontal_Mid_Orb_R"    "Frontal_Inf_Oper_L"   "Frontal_Inf_Oper_R"  
#>  [13] "Frontal_Inf_Tri_L"    "Frontal_Inf_Tri_R"    "Frontal_Inf_Orb_L"   
#>  [16] "Frontal_Inf_Orb_R"    "Rolandic_Oper_L"      "Rolandic_Oper_R"     
#>  [19] "Supp_Motor_Area_L"    "Supp_Motor_Area_R"    "Olfactory_L"         
#>  [22] "Olfactory_R"          "Frontal_Sup_Medial_L" "Frontal_Sup_Medial_R"
#>  [25] "Frontal_Med_Orb_L"    "Frontal_Med_Orb_R"    "Rectus_L"            
#>  [28] "Rectus_R"             "Insula_L"             "Insula_R"            
#>  [31] "Cingulum_Ant_L"       "Cingulum_Ant_R"       "Cingulum_Mid_L"      
#>  [34] "Cingulum_Mid_R"       "Cingulum_Post_L"      "Cingulum_Post_R"     
#>  [37] "Hippocampus_L"        "Hippocampus_R"        "ParaHippocampal_L"   
#>  [40] "ParaHippocampal_R"    "Amygdala_L"           "Amygdala_R"          
#>  [43] "Calcarine_L"          "Calcarine_R"          "Cuneus_L"            
#>  [46] "Cuneus_R"             "Lingual_L"            "Lingual_R"           
#>  [49] "Occipital_Sup_L"      "Occipital_Sup_R"      "Occipital_Mid_L"     
#>  [52] "Occipital_Mid_R"      "Occipital_Inf_L"      "Occipital_Inf_R"     
#>  [55] "Fusiform_L"           "Fusiform_R"           "Postcentral_L"       
#>  [58] "Postcentral_R"        "Parietal_Sup_L"       "Parietal_Sup_R"      
#>  [61] "Parietal_Inf_L"       "Parietal_Inf_R"       "SupraMarginal_L"     
#>  [64] "SupraMarginal_R"      "Angular_L"            "Angular_R"           
#>  [67] "Precuneus_L"          "Precuneus_R"          "Paracentral_Lobule_L"
#>  [70] "Paracentral_Lobule_R" "Caudate_L"            "Caudate_R"           
#>  [73] "Putamen_L"            "Putamen_R"            "Pallidum_L"          
#>  [76] "Pallidum_R"           "Thalamus_L"           "Thalamus_R"          
#>  [79] "Heschl_L"             "Heschl_R"             "Temporal_Sup_L"      
#>  [82] "Temporal_Sup_R"       "Temporal_Pole_Sup_L"  "Temporal_Pole_Sup_R" 
#>  [85] "Temporal_Mid_L"       "Temporal_Mid_R"       "Temporal_Pole_Mid_L" 
#>  [88] "Temporal_Pole_Mid_R"  "Temporal_Inf_L"       "Temporal_Inf_R"      
#>  [91] "Cerebelum_Crus1_L"    "Cerebelum_Crus1_R"    "Cerebelum_Crus2_L"   
#>  [94] "Cerebelum_Crus2_R"    "Cerebelum_3_L"        "Cerebelum_3_R"       
#>  [97] "Cerebelum_4_5_L"      "Cerebelum_4_5_R"      "Cerebelum_6_L"       
#> [100] "Cerebelum_6_R"        "Cerebelum_7b_L"       "Cerebelum_7b_R"      
#> [103] "Cerebelum_8_L"        "Cerebelum_8_R"        "Cerebelum_9_L"       
#> [106] "Cerebelum_9_R"        "Cerebelum_10_L"       "Cerebelum_10_R"      
#> [109] "Vermis_1_2"           "Vermis_3"             "Vermis_4_5"          
#> [112] "Vermis_6"             "Vermis_7"             "Vermis_8"            
#> [115] "Vermis_9"             "Vermis_10"           
#> 
#> $ba
#>  [1] "Left-Amygdala (53)"      "Left-BA10"              
#>  [3] "Left-BA11"               "Left-BA19"              
#>  [5] "Left-BA20"               "Left-BA21"              
#>  [7] "Left-BA22"               "Left-BA23"              
#>  [9] "Left-BA24"               "Left-BA25"              
#> [11] "Left-BA30"               "Left-BA31"              
#> [13] "Left-BA32"               "Left-BA34"              
#> [15] "Left-BA38"               "Left-BA39"              
#> [17] "Left-BA40"               "Left-BA44"              
#> [19] "Left-BA45"               "Left-BA46"              
#> [21] "Left-BA47"               "Left-BA6"               
#> [23] "Left-BA7"                "Left-BA8"               
#> [25] "Left-BA9"                "Left-Caudate (48)"      
#> [27] "Left-Fusiform (37)"      "Left-GlobPal (51)"      
#> [29] "Left-Hippocampus (54)"   "Left-Hypothalamus (55)" 
#> [31] "Left-Insula (13)"        "Left-NucAccumb (52)"    
#> [33] "Left-Parahip (36)"       "Left-PrimAuditory (41)" 
#> [35] "Left-PrimMotor (4)"      "Left-PrimSensory (1)"   
#> [37] "Left-PrimVisual (17)"    "Left-Putamen (49)"      
#> [39] "Left-SensoryAssoc (5)"   "Left-Thalamus (50)"     
#> [41] "Left-VisualAssoc (18)"   "Right-Amygdala (53)"    
#> [43] "Right-BA10"              "Right-BA11"             
#> [45] "Right-BA19"              "Right-BA20"             
#> [47] "Right-BA21"              "Right-BA22"             
#> [49] "Right-BA23"              "Right-BA24"             
#> [51] "Right-BA25"              "Right-BA30"             
#> [53] "Right-BA31"              "Right-BA32"             
#> [55] "Right-BA34"              "Right-BA38"             
#> [57] "Right-BA39"              "Right-BA40"             
#> [59] "Right-BA44"              "Right-BA45"             
#> [61] "Right-BA46"              "Right-BA47"             
#> [63] "Right-BA6"               "Right-BA7"              
#> [65] "Right-BA8"               "Right-BA9"              
#> [67] "Right-Caudate (48)"      "Right-Fusiform (37)"    
#> [69] "Right-GlobPal (51)"      "Right-Hippocampus (54)" 
#> [71] "Right-Hypothalamus (55)" "Right-Insula (13)"      
#> [73] "Right-NucAccumb (52)"    "Right-Parahip (36)"     
#> [75] "Right-PrimAuditory (41)" "Right-PrimMotor (4)"    
#> [77] "Right-PrimSensory (1)"   "Right-PrimVisual (17)"  
#> [79] "Right-Putamen (49)"      "Right-SensoryAssoc (5)" 
#> [81] "Right-Thalamus (50)"     "Right-VisualAssoc (18)"

Created on 2023-08-11 with reprex v2.0.2

yunshiuan commented 11 months ago

The atlas is meant to be hidden. If you want to access it directly, you can always do label4MRI:::list_brain_regions() (note the 3 colons instead of 2).

psychelzh commented 11 months ago

Sorry to make it confusing, but I think you have misunderstood me. The function list_brain_regions() is exported, so I do not need to use :::. As a matter of fact, label4MRI:::list_brain_regions() also leads to error:

label4MRI:::list_brain_regions()
#> Error in label4MRI:::list_brain_regions(): 找不到对象'label4mri_metadata'

Created on 2023-08-12 with reprex v2.0.2

If the data label4mri_metadata is meant to be hidden, we should even make it internal then, i.e., not in data folder, but in R folder. Hope this section would help understand what I mean.

And if label4mri_metadata is meant to be hidden, we do not need to document it in https://github.com/yunshiuan/label4MRI/blob/b990164850d4a0626aef5dcc9298f19b68ab0efe/R/label4mri_metadata.R

yunshiuan commented 11 months ago

I see. I misunderstood your question.

Not sure if there is an issue about R version, but label4MRI::list_brain_regions() works fine on my end with (R version 4.2.1).

label4MRI::list_brain_regions()
$aal
  [1] "Precentral_L"         "Precentral_R"         "Frontal_Sup_L"        "Frontal_Sup_R"       
  [5] "Frontal_Sup_Orb_L"    "Frontal_Sup_Orb_R"    "Frontal_Mid_L"        "Frontal_Mid_R"       
  [9] "Frontal_Mid_Orb_L"    "Frontal_Mid_Orb_R"    "Frontal_Inf_Oper_L"   "Frontal_Inf_Oper_R"  
 [13] "Frontal_Inf_Tri_L"    "Frontal_Inf_Tri_R"    "Frontal_Inf_Orb_L"    "Frontal_Inf_Orb_R"   
 [17] "Rolandic_Oper_L"      "Rolandic_Oper_R"      "Supp_Motor_Area_L"    "Supp_Motor_Area_R"   
 [21] "Olfactory_L"          "Olfactory_R"          "Frontal_Sup_Medial_L" "Frontal_Sup_Medial_R"
 [25] "Frontal_Med_Orb_L"    "Frontal_Med_Orb_R"    "Rectus_L"             "Rectus_R"            
 [29] "Insula_L"             "Insula_R"             "Cingulum_Ant_L"       "Cingulum_Ant_R"      
 [33] "Cingulum_Mid_L"       "Cingulum_Mid_R"       "Cingulum_Post_L"      "Cingulum_Post_R"     
 [37] "Hippocampus_L"        "Hippocampus_R"        "ParaHippocampal_L"    "ParaHippocampal_R"   
 [41] "Amygdala_L"           "Amygdala_R"           "Calcarine_L"          "Calcarine_R"         
 [45] "Cuneus_L"             "Cuneus_R"             "Lingual_L"            "Lingual_R"           
 [49] "Occipital_Sup_L"      "Occipital_Sup_R"      "Occipital_Mid_L"      "Occipital_Mid_R"     
 [53] "Occipital_Inf_L"      "Occipital_Inf_R"      "Fusiform_L"           "Fusiform_R"          
 [57] "Postcentral_L"        "Postcentral_R"        "Parietal_Sup_L"       "Parietal_Sup_R"      
 [61] "Parietal_Inf_L"       "Parietal_Inf_R"       "SupraMarginal_L"      "SupraMarginal_R"     
 [65] "Angular_L"            "Angular_R"            "Precuneus_L"          "Precuneus_R"         
 [69] "Paracentral_Lobule_L" "Paracentral_Lobule_R" "Caudate_L"            "Caudate_R"           
 [73] "Putamen_L"            "Putamen_R"            "Pallidum_L"           "Pallidum_R"          
 [77] "Thalamus_L"           "Thalamus_R"           "Heschl_L"             "Heschl_R"            
 [81] "Temporal_Sup_L"       "Temporal_Sup_R"       "Temporal_Pole_Sup_L"  "Temporal_Pole_Sup_R" 
 [85] "Temporal_Mid_L"       "Temporal_Mid_R"       "Temporal_Pole_Mid_L"  "Temporal_Pole_Mid_R" 
 [89] "Temporal_Inf_L"       "Temporal_Inf_R"       "Cerebelum_Crus1_L"    "Cerebelum_Crus1_R"   
 [93] "Cerebelum_Crus2_L"    "Cerebelum_Crus2_R"    "Cerebelum_3_L"        "Cerebelum_3_R"       
 [97] "Cerebelum_4_5_L"      "Cerebelum_4_5_R"      "Cerebelum_6_L"        "Cerebelum_6_R"       
[101] "Cerebelum_7b_L"       "Cerebelum_7b_R"       "Cerebelum_8_L"        "Cerebelum_8_R"       
[105] "Cerebelum_9_L"        "Cerebelum_9_R"        "Cerebelum_10_L"       "Cerebelum_10_R"      
[109] "Vermis_1_2"           "Vermis_3"             "Vermis_4_5"           "Vermis_6"            
[113] "Vermis_7"             "Vermis_8"             "Vermis_9"             "Vermis_10"           

$ba
 [1] "Left-Amygdala (53)"      "Left-BA10"               "Left-BA11"              
 [4] "Left-BA19"               "Left-BA20"               "Left-BA21"              
 [7] "Left-BA22"               "Left-BA23"               "Left-BA24"              
[10] "Left-BA25"               "Left-BA30"               "Left-BA31"              
[13] "Left-BA32"               "Left-BA34"               "Left-BA38"              
[16] "Left-BA39"               "Left-BA40"               "Left-BA44"              
[19] "Left-BA45"               "Left-BA46"               "Left-BA47"              
[22] "Left-BA6"                "Left-BA7"                "Left-BA8"               
[25] "Left-BA9"                "Left-Caudate (48)"       "Left-Fusiform (37)"     
[28] "Left-GlobPal (51)"       "Left-Hippocampus (54)"   "Left-Hypothalamus (55)" 
[31] "Left-Insula (13)"        "Left-NucAccumb (52)"     "Left-Parahip (36)"      
[34] "Left-PrimAuditory (41)"  "Left-PrimMotor (4)"      "Left-PrimSensory (1)"   
[37] "Left-PrimVisual (17)"    "Left-Putamen (49)"       "Left-SensoryAssoc (5)"  
[40] "Left-Thalamus (50)"      "Left-VisualAssoc (18)"   "Right-Amygdala (53)"    
[43] "Right-BA10"              "Right-BA11"              "Right-BA19"             
[46] "Right-BA20"              "Right-BA21"              "Right-BA22"             
[49] "Right-BA23"              "Right-BA24"              "Right-BA25"             
[52] "Right-BA30"              "Right-BA31"              "Right-BA32"             
[55] "Right-BA34"              "Right-BA38"              "Right-BA39"             
[58] "Right-BA40"              "Right-BA44"              "Right-BA45"             
[61] "Right-BA46"              "Right-BA47"              "Right-BA6"              
[64] "Right-BA7"               "Right-BA8"               "Right-BA9"              
[67] "Right-Caudate (48)"      "Right-Fusiform (37)"     "Right-GlobPal (51)"     
[70] "Right-Hippocampus (54)"  "Right-Hypothalamus (55)" "Right-Insula (13)"      
[73] "Right-NucAccumb (52)"    "Right-Parahip (36)"      "Right-PrimAuditory (41)"
[76] "Right-PrimMotor (4)"     "Right-PrimSensory (1)"   "Right-PrimVisual (17)"  
[79] "Right-Putamen (49)"      "Right-SensoryAssoc (5)"  "Right-Thalamus (50)"    
[82] "Right-VisualAssoc (18)"
psychelzh commented 11 months ago

If I attach the package, it will work. But if we do not want to make the data internal, we should change this line

https://github.com/yunshiuan/label4MRI/blob/b990164850d4a0626aef5dcc9298f19b68ab0efe/R/list_brain_regions.R#L25

as as.character(label4MRI::label4mri_metadata[[.template]]$label$Region_name) to make sure we do not need to attach this package to make the call to work.

It is not so elegant, so maybe make the data internal is better.

yunshiuan commented 11 months ago

Is there any reason that you want to use a library function/data without attaching the library?

psychelzh commented 11 months ago

I am using {targets} package, and if I could call the functions directly without attaching, it would make my call to tar_option_set() cleaner without too much packages in it. What's more, calling a function directly without attaching the package might be more elegant if I want to use it just in one line of my script. I do not need to edit my package attaching section of my script.

psychelzh commented 11 months ago

Do you accept PRs? I have made a PR #15, and I have installed that modified version, which works fine for me. Thanks for developing this helpful package.

yunshiuan commented 10 months ago

Hi, thanks for following up on this. Can you help convince me as to why this is a common issue other than when using the {targets} package?

psychelzh commented 10 months ago

I think there are two reasons:

  1. 'The atlas is meant to be hidden', indicating the dataset label4mri_metadata should not be stored as external data but internal data. I think Hadley's book chapter make this rule clear, and this blog also shows how to make a data both external and internal (maybe unnecessary for current package).
  2. While many users will call library() first when using a package, calling a function in a package using pkg::fun() package is also common (e.g., when we want to do some quick check in command window without attaching the package by library()). External data for a package cannot be accessed in such circumstances, unless the functions call these external data with its namespace (e.g., label4MRI::label4mri_metadata for current package). If the data treated internal, we do not need to refactor the codes.
yunshiuan commented 10 months ago

Sounds good. Merged. Thank you for bringing this up and proposing a merge request.