openneuropet / PET2BIDS

PET2BIDS helps you convert your PET data into BIDS! raw PET scanner files (e.g. ecat, dicom) and additional side files like .e.g excel sheets -- paper @JOSS https://doi.org/10.21105/joss.06067
https://pet2bids.readthedocs.io
MIT License
26 stars 20 forks source link

Feature Request / Bug: Ability to disable config file #291

Closed liningpan closed 8 months ago

liningpan commented 8 months ago

If I understand correctly, the sole purpose of the config file is to save the paths to dcm2niix and the template json file. This feature is extremely problematic/prohibitive for users who intend to use pypet2bids as a library.

  1. When using pypet2bids as a library, users calling the library programmatically should be given control over the template json file and dcm2niix.
  2. Additionally, saving the default template json file discovered during runtime has no benefit but causes a lot of problems for users who have multiple virtual environments or more ephemeral environments (bazel sandbox). When this saved path is not available, the actual cause is buried in a heap of exceptions because how modules are loaded in this project.
CPernet commented 8 months ago

Hi

Which config file are you talking about? after pip install nothing is needed.
The only, optional, file is the whatverscanner.txt which is intended to have a set of predetermined parameters that can be called all the time without having to specify them - but you can always fully specify all the parameters.

(no idea about temporary json file @bendhouseart will know)

liningpan commented 8 months ago

Here is an example. As soon as the dec2niix4pet module is imported, it will create the .pet2bidsconfig file in your home directory to save the DEFAULT_METADATA_JSON path.

https://github.com/openneuropet/PET2BIDS/blob/a0d95ac42665ace6e6b918b24a93e23ad27e5b96/pypet2bids/pypet2bids/dcm2niix4pet.py#L67-L87

https://github.com/openneuropet/PET2BIDS/blob/a0d95ac42665ace6e6b918b24a93e23ad27e5b96/pypet2bids/pypet2bids/helper_functions.py#L855-L889

bendhouseart commented 8 months ago

If I understand correctly, the sole purpose of the config file is to save the paths to dcm2niix and the template json file. This feature is extremely problematic/prohibitive for users who intend to use pypet2bids as a library.

Yes, you are correct as the config file was primarily developed so that users who are unable to put dcm2niix on their path are able to use this software. Later, it's use got expanded to handle other cases. In this example having a template/config such that a user could set "unchanging" values for their lab/analysis such as the Units or Description of plasma_radioactivity or metabolite_parent_fraction once in a "transparent" way. The alternative was to have users provide these values via kwargs or including them in a spreadsheet. My users balked at such a thing and that's why we settled on the template.json file.

When using pypet2bids as a library, users calling the library programmatically should be given control over the template json file and dcm2niix.

I think there are two separate (but related) issues going on here.

  1. You would have control over this by using the helper_functions.check_pet2bids_config if it didn't default to looking for the config at $HOME/.pet2bidsconfig.
  2. helper_functions.modify_config_file seems to default to the template file that it finds (or creates) at $HOME/.pet2bidsconfig too. If it's unable to create that file you run into the file not found issue.

Do you have any interest in using the config file to set paths to dcm2nii or to a template file?

Additionally, saving the default template json file discovered during runtime has no benefit but causes a lot of problems for users who have multiple virtual environments or more ephemeral environments (bazel sandbox). When this saved path is not available, the actual cause is buried in a heap of exceptions because how modules are loaded in this project.

This seems like an oversight on my part as I hadn't considered the inability of this library to create a file at the users home dir. But it seems like you've uncovered this so it needs to be more robust.

bendhouseart commented 8 months ago

I should note that: we use config files with the python-dotenv module to read them so there's nothing stopping us from moving over checking the environment for the values we're hoping to pick up.

I think that's going to be how we move forward with this, but I'd be curious to hear from you about how you would intend to make use (or not use) the config or setting a template file before I start to implement that sort of change so I'm able to solve your issue @liningpan

bendhouseart commented 8 months ago

Following up with this, below we can see what happens when no .pet2bidsconfig file exists and isn't present in the environment:

In [2]: x = check_pet2bids_config()
2024-04-02 18:02:40,496 - pypet2bids - ERROR - Unable to locate dcm2niix executable at None Set DCM2NIIX_PATH variable in /Users/galassiae/.pet2bidsconfig or export DCM2NIIX_PATH=/path/to/dcm2niix into your environment variables. (helper_functions.py:1034)

Here we can see what is returned if we have it set in the environment, but no config file exists:

In [3]: x = check_pet2bids_config('HOME')
2024-04-02 18:03:13,088 - pypet2bids - WARNING - Found HOME in environment variables as /Users/galassiae, but no .pet2bidsconfig file found at /Users/galassiae/.pet2bidsconfig (helper_functions.py:1018)
In [4]: x
Out[4]: '/Users/galassiae'

Going to kick this off as a PR to see if it breaks anything.