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.

Sign up for GitHub

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

Jump to bottom

Ensure that the straightening.cache file is output to the PWD during template registration #4156

Merged
merged 7 commits into from Jul 13, 2023
Merged

Ensure that the straightening.cache file is output to the PWD during template registration #4156

merged 7 commits into from Jul 13, 2023

Conversation

joshuacwnewton
Copy link
Member

Description

This PR adds a check that tests for the existence of straightening files generated by sct_register_to_template and sct_label_vertebrae. Notably, this test initially fails for sct_register_to_template due to #4138.

Then, this PR updates the cache-saving behavior so that A) the cache file is first saved to the temporary directory used by both scripts, then B) the cache file is copied to the output directory along with the other straightening files.

Linked issues

Fixes #4138.

@joshuacwnewton
testing/: Use tmp_path for registration test
b3f917f 
This is a necessary prerequisite for testing whether the straightening files were created in the correct directory. (Because, if we don't use a tmp_path and check for the files in the `sct_testing_data` directory, then we could get a false negative due to other tests creating these exact same straightening files.)
@joshuacwnewton
testing/: Check for straightening files in reg and label tests
cfaa661 
The `sct_label_vertebrae` test will pass, but the `sct_register_to_template` test will fail, because `straightening.cache` is improperly output to the working directory.
@joshuacwnewton
sct_register_to_template: Copy cache file to output dir
518eec2 
Previously, the `cachefile` path made it so that `cache_save()` saved the file to `curdir` (i.e. the working directory).

After this change, `curdir` is only used to check whether an *input* cache file exists. But, for saving, we now A) save to the temporary directory and B) copy it to the output directory.
@joshuacwnewton
sct_label_vertebrae: Mimic the logic used in `sct_register_to_templ…
13fd59b 
…ate`

Instead of saving the cache file directly to the output directory, we now follow the same logic as the registration script:

A) save to the temporary directory and B) copy it to the output directory.
@joshuacwnewton joshuacwnewton added bug category: fixes an error in the code sct_register_to_template context: labels Jul 12, 2023
@joshuacwnewton joshuacwnewton added this to the 6.0 milestone Jul 12, 2023
@joshuacwnewton joshuacwnewton self-assigned this Jul 12, 2023
@mguaypaq
Save the straightening cache
b44f463 
Later code (namely, `generate_output_file`) expects it to be present.
@mguaypaq
Inline fname_cache
31a62b9 
Other file names are spelled out, and it wasn't being used
consistently anyway.
mguaypaq
mguaypaq approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine, although our tests caught a corner case, which I think I fixed.

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jul 13, 2023 •
edited

Hrm. Just to make sure that I understand why the test failed:

  • One of the sct_label_vertebrae tests attempted to re-use some existing straightening files from the PWD.
  • But, the "reuse existing straightening files" code only copied the image files, and not the straightening.cache file.
  • So, when the code later on tries to copy all of the straightening files from the tmpdir to the output directory (as per the purpose of this PR), it fails, because straightening.cache was never generated in the tmpdir (due to cache_save never being run).
  • Your fix ensures that the pre-existing straightening.cache file in the PWD gets copied to the tmpdir, which will later on get copied from the tmpdir to the output directory.

If that's the case, then the fix from b44f463 makes perfect sense to me. Thank you for this!

(Though, as a side note, I didn't realize that the sct_label_vertebrae tests were reusing straightening files from previous tests!! I'm thankful that this behavior caught the bug I almost introduced, but it means that one of the sct_label_vertebrae tests is still saving files to the PWD. Hrm...)

@mguaypaq
Copy link
Member

Hrm. Just to make sure that I understand why the test failed: [...]

Yep! That's it exactly.

(Though, as a side note, I didn't realize that the sct_label_vertebrae tests were reusing straightening files from previous tests!! I'm thankful that this behavior caught the bug I almost introduced, but it means that one of the sct_label_vertebrae tests is still saving files to the PWD. Hrm...)

I had the same worry, but I don't think that's the case. If you look at the test in question, it's a single test that calls sct_label_vertebrae with the same images 3 times, so it makes sense that it would re-use the straightening results (which are the same for all 3 calls).

Wait, actually, you're right that sct_label_vertebrae looks for straightening.cache in the PWD, so that's still fishy.

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jul 13, 2023 •
edited

I guess perhaps we could merge this PR as-is for now, but tackle this further as part of #2911?

i.e. when we change the test suite so that tests are inherently run in a temporary directory, we make sure to explicitly test these scripts' ability to load cached files (as opposed to testing this behavior implicitly, as we're doing now.)

@mguaypaq
Copy link
Member

Well, we can't really claim to be fixing #4138 unless we understand what's going on here, I think.

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jul 13, 2023 •
edited

Ah, sorry. I thought we understood what was going on?

  • #4138 focuses on whether sct_register_to_template is outputting the straightening.cache file to the -ofolder directory.
  • But, the new test failures were due to one of the straightening-file-generating tests not using -ofolder at all (presumably the test test_sct_label_vertebrae_initfile_qc_no_checks), resulting in all 4 straightening files being output to the PWD (sct_testing_data), causing those files to later be re-used when further sct_label_vertebrae tests were run.

So, I figured that the former issue had been solved by this PR, while the latter issue was more of a "tests don't clean up after themselves" issue (hence #2911).

@mguaypaq
Copy link
Member

Ah ok, makes sense.

@joshuacwnewton joshuacwnewton merged commit dde8d09 into master Jul 13, 2023
24 checks passed
@joshuacwnewton joshuacwnewton deleted the jn/4138-use-output-path-for-cache-file branch July 13, 2023 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mguaypaq mguaypaq mguaypaq approved these changes

Assignees

@joshuacwnewton joshuacwnewton

Labels
bug category: fixes an error in the code sct_register_to_template context:
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.

straightening.cache file is output to the working directory, even when -ofolder is used
2 participants
@joshuacwnewton @mguaypaq