[JOSS review] Documentation review

[nbeliy]

• [x] Lack of command description for online help (here example of dcm2niix4pet, but issue is present in other commands):

>dcm2niix4pet --help
usage: dcm2niix4pet [-h] [--metadata-path [METADATA_PATH]]
                    [--translation-script-path TRANSLATION_SCRIPT_PATH]
                    [--destination-path DESTINATION_PATH] [--tempdir TEMPDIR] [--kwargs [KWARGS ...]]
                    [--silent] [--show-examples] [--set-dcm2niix-path SET_DCM2NIIX_PATH]
                    [--set-default-metadata-json SET_DEFAULT_METADATA_JSON] [--trc TRC] [--run RUN]
                    [--rec REC]
                    [folder]

positional arguments:
  folder                Folder path containing imaging data

• [x] In readthesedocs, no mentioning of PET JSON updater, a function(s) that you explicitly describe in paper.
• [x] Lack of documentation CLI commands installed by pip, namely all updatejson* functions
• [x] Docstring for update_json_pet_file.check_meta_radio_inputs needs to be rewrite (author does not recall everything it does).
• [x] Repository don't have tags/releases
• [x] Repository do not follow PEP8, flake8 finds > 1000 errors. Outside purely stylistic ones, I saw few F541 f-string is missing placeholders, and few F405 'pathlib' may be undefined, that looked dangerous.

[bendhouseart]

Repository don't have tags/releases

Is tagging in git a requirement? We haven't used tagging as pypi and our pyproject.toml to keep track of version and minor fixes.

Repository do not follow PEP8, flake8 finds > 1000 errors. Outside purely stylistic ones, I saw few F541 f-string is missing placeholders, and few F405 'pathlib' may be undefined, that looked dangerous.

Is this a requirement? It's easy enough to fix, but I don't want to apply it until after this review completes and I've closed out some existing PR's as it changes every file and makes PR's difficult to evaluate. See here -> https://github.com/openneuropet/PET2BIDS/pull/283/files

Would you be satisfied for now if I added formatting to the main Makefile and later updated the CI to run the command as well as a precommit hook or similar?

# Quickly run black on all python files in this repository, local version of the pre-commit hook
black:
    @for file in `find . -name "*.py"`; do \
        black $$file; \
    done

[bendhouseart]

I saw few F541 f-string is missing placeholders

flake8 doesn't seem to like the following:

 logging.warning(
                            f"Autosampled .bld input for {auto['name']} has {auto['sample_length']} rows"
                            f" and Manually sampled input has {manual['sample_length']} rows. Autosampled blood "
                            f"files should have more rows than manually sampled input files. Check .bld inputs."
                        )

But I would argue that it's being a bit pedantic as that's what my linter in Pycharm suggests. I guess the alternative would be:

                            f"Autosampled .bld input for {auto['name']} has {auto['sample_length']} rows"
                            f" and Manually sampled input has {manual['sample_length']} rows. Autosampled blood "
                            "files should have more rows than manually sampled input files. Check .bld inputs."
                        )

Which looks worse to me.

[nbeliy]

Hi @bendhouseart ,

The tags are not a requirement, but you will be asked to create a release for the publication of JOSS. It also would facilitate the reference of the pet2bids. But I will not push this point.

For the flake8, I didn't checked the f-string issues, just saw that there are. So if you confirm that none of these "malformed" f-strings will cause problems (for example if your linter do not complains), I'll accept it.

I'm more worried for the undefined references in the code. This looks much more scary. But again, if you confirm that these references are not causing problems, I'll accept it. Well, maybe I'll cross-check, just in case)

[bendhouseart]

I'm on the fence about whether we should tag more as we end up making so many small changes and fixes as new data comes in. Pushing directly to PyPi has made the most sense for us in distributing this to our users that we've just stuck with it. That said, happy to tag with JOSS or whatever you ask pending publication.

I'm more worried for the undefined references in the code. This looks much more scary. But again, if you confirm that these references are not causing problems, I'll accept it. Well, maybe I'll cross-check, just in case)

Yeah, let's fix these as they might be bad, they're probably okay, but they're popping up in an older (not as heavily used if at all) sections of the code.

Lack of command description for online help (here example of dcm2niix4pet, but issue is present in other commands):

Maintaining web links at the CLI level isn't something I particularly want to delve into at this moment. However, would updating the PyPi page with links to the readthedocs page as well as a rendering of the README.md be a good compromise? See:

Then anyone with a search engine should be able to locate the documentation and source by reaching either PyPi or the github repo?

I'll update the other bits of documentation as you pointed out in this issue and make a PR that you can sign off on.

[nbeliy]

Maintaining web links at the CLI level isn't something I particularly want to delve into at this moment. However, would updating the PyPi page with links to the readthedocs page as well as a rendering of the README.md be a good compromise? See:

Sorry, used old vocabulary. When I said online help, I meant the help you get using --help option. When you initialize the argument parser, you just need to add one line of what the given program does. For example:

parser = argparse.ArgumentParser(
       formatter_class=argparse.RawDescriptionHelpFormatter,
       description="Converts DICOM PET images to Nifti and extract metadata following BIDS",
       epilog=epilog)

[nbeliy]

Yeah, let's fix these as they might be bad, they're probably okay, but they're popping up in an older (not as heavily used if at all) sections of the code.

I also stumbled on undefined variable in dicom_convert.py, line 240. zfill used here alone, and few lines above is used as nifti_run_number.zfill.

[bendhouseart]

Alright, added the update pet stuff to the read the docs site and descriptions for all of the cli's. I think I just need to touch up the readme slightly and make the import's happy with flake*

I also stumbled on undefined variable in dicom_convert.py, line 240. zfill used here alone, and few lines above is used as nifti_run_number.zfill.

Should just delete this file as it was the original converter and hasn't been used forever.