JPETTomography / j-pet-format-converter

Apache License 2.0
1 stars 3 forks source link

PET DICOM attributes + Tests #10

Closed aistrych closed 1 year ago

aistrych commented 1 year ago
graag commented 1 year ago

BTW in the tests shouldn't we use the original SIEMENS DICOM files as references?

aistrych commented 1 year ago
  • I don't think it is good practice to store all the test data into the Git repository, because it makes it heavier and slower. I would store those files somewhere and have the tests download them.

Ok, we can move test data (files inside tests/inputs directory) to an external server. @graag suggested that we should put them on UJ server in Krakow (used by other JPET projects). @wkrzemien Can you please do that? I believe I don't have access there.

wkrzemien commented 1 year ago

@

  • I don't think it is good practice to store all the test data into the Git repository, because it makes it heavier and slower. I would store those files somewhere and have the tests download them.

Ok, we can move test data (files inside tests/inputs directory) to an external server. @graag suggested that we should put them on UJ server in Krakow (used by other JPET projects). @wkrzemien Can you please do that? I believe I don't have access there.

@Astrych Ok, point me what exactly should be copied

aistrych commented 1 year ago

@wkrzemien Everything inside tests/inputs/dicoms and interfile (header + image) from tests/inputs/interfiles (do not copy metadata json files)

https://github.com/Astrych/j-pet-format-converter/tree/develop/tests/inputs

wkrzemien commented 1 year ago

@Astrych http://sphinx.if.uj.edu.pl/~rakoczy/FormatConverterTests/

acoussat commented 1 year ago

Hi! Two comments:

aistrych commented 1 year ago

Hi! Two comments:

  • the tests seem to fail on my copy of the code. When running pytest, the test test_read_interfile_image fails with error:
ValueError: incompatible dimensions for cross product
(dimension must be 2 or 3)

Am I doing anything wrong?

  • You can find some references about how we download tests for the files here or here.

Yeah, I've made typo in that test (it should be ipp instead of iop). Now it works. I have no access to total-body-tools and castor-examples unfortunately. @wkrzemien Can you do something about that?

acoussat commented 1 year ago

Hi! Two comments:

  • the tests seem to fail on my copy of the code. When running pytest, the test test_read_interfile_image fails with error:
ValueError: incompatible dimensions for cross product
(dimension must be 2 or 3)

Am I doing anything wrong?

  • You can find some references about how we download tests for the files here or here.

Yeah, I've made typo in that test (it should be ipp instead of iop). Now it works.

Can you push the fix so that I can test it?

I have no access to total-body-tools and castor-examples unfortunately. @wkrzemien Can you do something about that?

Here are the relevant snippets of code:

def download_file(
    filename,
    out_folder,
    url='http://sphinx.if.uj.edu.pl/~rakoczy/totalBody/'
):
  url = url + filename
  result = requests.get(url)
  with open(out_folder + filename, 'wb') as out_file:
    out_file.write(result.content)
@pytest.fixture
def data_directory(pytestconfig):
    data_dir = pytestconfig.getoption("data_dir")
    if data_dir is not None:
        return data_dir

    default_path = os.path.join("castor-develop-pipeline", "pipeline")
    if not os.path.isdir(default_path):
        cwd = os.getcwd()
        gpg_file = os.path.join(cwd, "castor-ci-data.gpg")
        zip_file = os.path.join(cwd, "castor-ci-data.zip")

        request.urlretrieve("http://sphinx.if.uj.edu.pl/~dtrybek/castor_test_data/castor-ci-data.gpg", gpg_file)

        result = subprocess.run(["gpg", "--output", zip_file, "--decrypt", "--batch", "--passphrase",
                                 "Axp2VsJcOJINN03zzbBv", gpg_file])
        assert result.returncode == 0, result.stderr

        with zipfile.ZipFile(zip_file, 'r') as zip_ref:
            zip_ref.extractall(".")

    return default_path

I don't know if it helps…

aistrych commented 1 year ago

@acoussat I've pushed it already yesterday. It should work.

Thanks, those snippets should be enough.

acoussat commented 1 year ago

@acoussat I've pushed it already yesterday. It should work.

Sorry, I missed it. Thank you, it works fine now.

wkrzemien commented 1 year ago

@Astrych please, remove the unnecessary test files that are now downloaded from the server. It would be good to remove it completely from git history eg. using git filter-branch

wkrzemien commented 1 year ago

Also, add a minimum description of the PR, e.g. what it contains. Otherwise it looks very good!

graag commented 1 year ago

@wkrzemien I think its ready.

BTW afterwards we should probably merge develop into master ...

wkrzemien commented 1 year ago

Ok thank you. @Astrych Which files should I remove from the sphinx server?

aistrych commented 1 year ago

@wkrzemien example_pt folder: http://sphinx.if.uj.edu.pl/~rakoczy/FormatConverterTests/inputs/dicoms/example_pt/

wkrzemien commented 1 year ago

Files removed