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.

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

image.convert function is called incorrectly, causing error during registration #3282

Closed
joshuacwnewton opened this issue Mar 14, 2021 · 3 comments · Fixed by #3288
Closed
Assignees
Labels
bug category: fixes an error in the code priority:HIGH SCT API: registration context: user requested Raised by user on the SCT forum/email/GitHub. Be sure to notify them when fixed in a release.
Milestone

Comments

@joshuacwnewton
Copy link
Member

joshuacwnewton commented Mar 14, 2021

A user on the forum is reporting an error: https://forum.spinalcordmri.org/t/register-template-to-mt1/654

The bug is from this line of code:

https://github.com/neuropoly/spinalcordtoolbox/blob/0b7be07b7e64a57a303824e85930fbceb8b890f0/spinalcordtoolbox/registration/register.py#L373

The usage of this function doesn't match its parameters:

https://github.com/neuropoly/spinalcordtoolbox/blob/0b7be07b7e64a57a303824e85930fbceb8b890f0/spinalcordtoolbox/image.py#L1341-L1348

I believe image.convert came about in #2871/#2812. I'll dig into that and find out what the correct usage should be.

@joshuacwnewton joshuacwnewton added bug category: fixes an error in the code priority:HIGH SCT API: registration context: labels Mar 14, 2021
@joshuacwnewton joshuacwnewton added this to the 5.2.1 milestone Mar 14, 2021
@joshuacwnewton joshuacwnewton self-assigned this Mar 14, 2021
@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Mar 15, 2021

Ah, I see what happened. There are multiple functions named convert in SCT:

  • image.convert
  • sct_convert.convert

When sct_image was refactored in #2812, the function sct_convert.convert was mistaken for image.convert.

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Mar 15, 2021

The two functions are basically identical. sct_convert.convert simply adds file I/O to image.convert, which doesn't really justify a function.

https://github.com/neuropoly/spinalcordtoolbox/blob/0b7be07b7e64a57a303824e85930fbceb8b890f0/spinalcordtoolbox/scripts/sct_convert.py#L70-L79

Because scripts should be importing from API (image.convert) and not the CLI (sct_convert.convert), I'm going to remove sct_convert.convert and replace all calls with image.convert. That way, there will be no confusion in the future.

@Drulex
Copy link
Collaborator

Drulex commented Mar 16, 2021

Woops this would be my fault.

The two functions are basically identical. sct_convert.convert simply adds file I/O to image.convert, which doesn't really justify a function.

The initial motivation to keep sct_convert.convert() was to maintain backwards compatibility while using the refactored image.convert() API internally until we are ready to break the interface. (some scripts were importing sct_convert.py from scripts at that point, but this was fixed in #2966)

@joshuacwnewton joshuacwnewton added this to Issues: To do in 5.3.0 Roadmap via automation Mar 30, 2021
@joshuacwnewton joshuacwnewton moved this from Issues: To do to Issues/PRs: Done in 5.3.0 Roadmap Mar 30, 2021
@joshuacwnewton joshuacwnewton added the user requested Raised by user on the SCT forum/email/GitHub. Be sure to notify them when fixed in a release. label Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category: fixes an error in the code priority:HIGH SCT API: registration context: user requested Raised by user on the SCT forum/email/GitHub. Be sure to notify them when fixed in a release.
Projects
No open projects
5.3.0 Roadmap
Issue/PR: Done
Development

Successfully merging a pull request may close this issue.

2 participants