fixing typing as string issue #6

yibeichan · 2023-05-19T17:26:56Z

Dorota and I tested the converter with ExtractROI from fsl.utils and found that typing in the input specs was a string, such as "File" and "str".

input_fields = [
    (
        "in_file",
        "File",
        {
            "help_string": "input file",
            "argstr": "{in_file}",
            "mandatory": True,
            "position": 0,
        },
    ),
    (
        "roi_file",
        "str",
        {
            "help_string": "output file",
            "argstr": "{roi_file}",
            "position": 1,
            "output_file_template": "{in_file}_trim",
        },
    ),

By uncommenting the following in the write_task() function, we get the correct typing

for tp_repl in self.TYPE_REPLACE:
      spec_str = spec_str.replace(*tp_repl)

Correct typing specs.File and str as the following

input_fields = [
    (
        "in_file",
        specs.File,
        {
            "help_string": "input file",
            "argstr": "{in_file}",
            "mandatory": True,
            "position": 0,
        },
    ),
    (
        "roi_file",
        str,
        {
            "help_string": "output file",
            "argstr": "{roi_file}",
            "position": 1,
            "output_file_template": "{in_file}_trim",
        },
    ),

yibeichan · 2023-05-23T03:47:33Z

@djarecka ah interesting, I put the ExtractROI into the example-specs and found the pytest failed. It might be something related to this function itself. We can discuss it this Friday.
Also, ants_registration_Registration now has TypeError: '<' not supported between instances of 'type' and 'str' now. Not sure whether my uncommenting causes it. @tclose Was this the reason you originally commented out those two lines of code?

tclose · 2023-05-23T04:02:39Z

Yes, I think there was an issue with 'str' being converted to str where it wasn't supposed to be. Could we be more specific with that replacement and only target the datatype lines?

tclose · 2023-05-23T04:30:22Z

While still a bit brittle maybe the regex could be

TYPE_REPLACE = [
    (r"(?<=        )'(bool|str)'(?=,$)", r"\1"),
    (r"(?<=        )'(File|Multi.*put.*)'(?=,$)", r"specs.\1"),
]

and then use re.sub(tpl_repl[0], tp_repl[1], spec_str)

yibeichan · 2023-05-31T17:36:48Z

Hi @tclose sorry for this late reply. @djarecka and I discussed the issue today, and we found out why ants_registration_Registration failed the test after I uncommented those two lines.

for tp_repl in self.TYPE_REPLACE:
      spec_str = spec_str.replace(*tp_repl)

If we comment out those two lines, this line from nipype will be converted to

(
        "float",
        "bool",
        {
            "help_string": "Use float instead of double for computations.",
            "argstr": "--float {float}",
        },
    ),

If we uncomment those two lines, it will be converted to

(
        float,
        bool,
        {
            "help_string": "Use float instead of double for computations.",
            "argstr": "--float {float}",
        },
    ),

now float becomes a type and will cause errors in this line

Now I understand what you mean by using regex to specify the replacement.

yibeichan · 2023-05-31T18:56:40Z

@djarecka I found out why the ExtractROI test failed.
This line compares our pydra task specs with nipype specs of ExtractROI. So we got

assert ['in_file', 'roi_file', 't_min', 't_size', 'x_min', 'x_size', 'y_min', 'y_size', 'z_min', 'z_size'] == ['crop_list', 'in_file', 'output_type', 'roi_file', 't_min', 't_size', 'x_min', 'x_size', 'y_min', 'y_size', 'z_min', 'z_size']

which is like

assert pydra_task.input_spec.fields == nipype_trait_names

But crop_list and output_type are not supposed to appear on the right side (nipype_trait_names).
So we can add those two to this list, which is used in this line

I can add it once we think this is the best solution.

tclose · 2023-05-31T23:13:21Z

Hi @tclose sorry for this late reply. @djarecka and I discussed the issue today, and we found out why ants_registration_Registration failed the test after I uncommented those two lines.
for tp_repl in self.TYPE_REPLACE:
      spec_str = spec_str.replace(*tp_repl)
If we comment out those two lines, this line from nipype will be converted to
(
        "float",
        "bool",
        {
            "help_string": "Use float instead of double for computations.",
            "argstr": "--float {float}",
        },
    ),
If we uncomment those two lines, it will be converted to
(
        float,
        bool,
        {
            "help_string": "Use float instead of double for computations.",
            "argstr": "--float {float}",
        },
    ),
now float becomes a type and will cause errors in this line

Now I understand what you mean by using regex to specify the replacement.

Yes, that was it. Seeing that example, the regex I suggested still wouldn't work, maybe this one would though

TYPE_REPLACE = [
    (r"(?<=        )'(bool|str)'(?=,\n        {)", r"\1"),
    (r"(?<=        )'(File|Multi.*put.*)'(?=,\n        {)", r"specs.\1"),
]

yibeichan · 2023-05-31T23:48:00Z

hi!! I fixed the typing problem with your regex logic, but I wrote something differently (I mean I asked chatgpt to write it).

# apply each replacement in TYPE_REPLACE
for old, new in self.TYPE_REPLACE:
    # add a comma and newline to the old string, to match only at the end of lines
    old += ",\n"
    # add a comma and newline to the new string, to replace with
    new += ",\n"
    # use re.sub to replace old with new, in spec_str
    spec_str = re.sub(old, new, spec_str)

we still failed the test but only ExtractROI, which is a different issue. ants_registration_Registration is good now.

tclose · 2023-05-31T23:50:53Z

hi!! I fixed the typing problem with your regex logic, but I wrote something differently (I mean I asked chatgpt to write it).
# apply each replacement in TYPE_REPLACE
for old, new in self.TYPE_REPLACE:
    # add a comma and newline to the old string, to match only at the end of lines
    old += ",\n"
    # add a comma and newline to the new string, to replace with
    new += ",\n"
    # use re.sub to replace old with new, in spec_str
    spec_str = re.sub(old, new, spec_str)
we still failed the test but only ExtractROI, which is a different issue. ants_registration_Registration is good now.

That's good that it works but I don't get the point of the ChatGPT code, if you want your regex to only match the end of lines you can just use the $ symbol

yibeichan · 2023-05-31T23:57:55Z

hi!! I fixed the typing problem with your regex logic, but I wrote something differently (I mean I asked chatgpt to write it).
# apply each replacement in TYPE_REPLACE
for old, new in self.TYPE_REPLACE:
    # add a comma and newline to the old string, to match only at the end of lines
    old += ",\n"
    # add a comma and newline to the new string, to replace with
    new += ",\n"
    # use re.sub to replace old with new, in spec_str
    spec_str = re.sub(old, new, spec_str)
we still failed the test but only ExtractROI, which is a different issue. ants_registration_Registration is good now.
That's good that it works but I don't get the point of the ChatGPT code, if you want your regex to only match the end of lines you can just use the $ symbol

so the code does the replacement only when the strings are followed by a comma and a newline

codecov-commenter · 2023-05-31T23:59:02Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.36 🎉

Comparison is base (a3d984e) 79.55% compared to head (88feb32) 80.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   79.55%   80.92%   +1.36%     
==========================================
  Files           4        4              
  Lines         362      367       +5     
==========================================
+ Hits          288      297       +9     
+ Misses         74       70       -4

Impacted Files	Coverage Δ
nipype2pydra/task.py	`80.83% <100.00%> (+1.60%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

yibeichan · 2023-06-01T00:00:19Z

@djarecka I found out why the ExtractROI test failed. This line compares our pydra task specs with nipype specs of ExtractROI. So we got
assert ['in_file', 'roi_file', 't_min', 't_size', 'x_min', 'x_size', 'y_min', 'y_size', 'z_min', 'z_size'] == ['crop_list', 'in_file', 'output_type', 'roi_file', 't_min', 't_size', 'x_min', 'x_size', 'y_min', 'y_size', 'z_min', 'z_size']
which is like
assert pydra_task.input_spec.fields == nipype_trait_names
But crop_list and output_type are not supposed to appear on the right side (nipype_trait_names). So we can add those two to this list, which is used in this line

I can add it once we think this is the best solution.

I just tested, adding crop_list and output_type to this list solved the problem.

Also I removed the test input here because we don't have any test data.
now all tests are passed.

tclose · 2023-06-01T00:25:13Z

But isn't float followed by comma and a newline in the ANTs registration case

(
        "float",
        "bool",
        {
            "help_string": "Use float instead of double for computations.",
            "argstr": "--float {float}",
        },
    ),

tclose · 2023-06-01T00:26:21Z

I was thinking that including the following line up until the open brace '{' would differentiate between the two

yibeichan · 2023-06-01T02:58:12Z

But isn't float followed by comma and a newline in the ANTs registration case

(
        "float",
        "bool",
        {
            "help_string": "Use float instead of double for computations.",
            "argstr": "--float {float}",
        },
    ),

yes, in this case, we want to convert "bool" to bool while keeping "float" as "float".
oh, I typed something wrong... I mean the ChatGPT code does the replacement only when a string follows a comma and a new line...
you mean we want to use xxx, +\n +{ as an identifier instead of , +\n xxx

tclose · 2023-06-01T03:01:12Z

you mean we want to use xxx, +\n +{ as an identifier instead of , +\n xxx

Yes, this is what I was thinking, maybe explicitly typing out the full indentation so it is guaranteed to be at the top level

yibeichan · 2023-06-01T03:02:14Z

we probably don't want to use { because in ants_registration_Registration we have something like the following, where { is not next to the trait type

(
        "use_histogram_matching",
        ty.Any,
        True,
        {"help_string": "Histogram match the images before registration."},
    ),
    (
        "interpolation",
        ty.Any,
        "Linear",
        {"help_string": "", "argstr": "{interpolation}"},
    ),
    ("interpolation_parameters", ty.Any, {"help_string": ""}),
    (
        "write_composite_transform",
        bool,
        False,
        {
            "help_string": "",
            "argstr": "--write-composite-transform {write_composite_transform}",
        },
    ),

tclose · 2023-06-01T03:06:51Z

It might be better to target the attrs.field style syntax then

tclose · 2023-06-01T03:07:40Z

Or you could do a negative look-behind for

(?<!\(\n        )

yibeichan · 2023-06-01T03:15:47Z

It might be better to target the attrs.field style syntax then

sorry, not very familiar with this. what do you mean by "target the attrs.field style syntax"

a negative look-behind for (?<!\(\n ) is easier for me to understand. I can do this one. But I still want to know what the attrs.field style syntax is, haha

tclose · 2023-06-01T03:20:20Z

Otherwise written as attr.ib, is an optional way to declare the Pydra interface, see the output specs in the docs as an example

yibeichan · 2023-06-01T03:33:28Z

Otherwise written as attr.ib, is an optional way to declare the Pydra interface, see the output specs in the docs as an example

ah, make sense. okay, let's choose one. attr.ib or negative look-behind?

tclose · 2023-07-13T04:23:44Z

I'm thinking a better way to handle this would be to escape the types in a pair of "#", e.g. "#float#", which we can be confident of stripping out after the code is generated, rather than messing around with regexes

yibeichan · 2023-07-16T14:15:54Z

ah! hello!! and sorry for not getting back to this issue earlier. I was traveling and moving for the past few weeks. I'll work on it the coming week!!

…

On Thu, Jul 13, 2023 at 12:23 AM Tom Close ***@***.***> wrote: I'm thinking a better way to handle this would be to escape the types in a pair of "#", e.g. "#float#", which we can be confident of stripping out after the code is generated, rather than messing around with regexes — Reply to this email directly, view it on GitHub <#6 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE3VE4AWPXRXIWULUFPE3NLXP5Z5VANCNFSM6AAAAAAYIBODSY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

yibeichan · 2023-07-24T14:54:43Z

hello! (after another week...) okay, your idea inspired me, but I didn't add a pair of # for types. Instead, I add a prefix 'TYPE_' -> 'TYPE_list'. And it worked! In this case, we don't have to touch regex
okay, this PR requires a reviewer, but I can't assign anyone to review... I guess you will review it? then this problem solved?

tclose

Nice idea with the "TYPE_" prefix it is much more explicit. All looks good to me :)

fixing typing as string issue

f960b93

fix typing converting issue

369524a

add inbuilt nipype trait names

e14d028

yibeichan added 2 commits July 24, 2023 10:17

Merge branch 'nipype:main' into convert-fsl-1

7013470

fix the converting problem by adding prefix

3d29e9a

remove extra print

88feb32

remove extra blank lines

40bd802

tclose approved these changes Jul 24, 2023

View reviewed changes

tclose merged commit b6cf4d2 into nipype:main Jul 24, 2023

yibeichan deleted the convert-fsl-1 branch July 25, 2023 01:24

fixing typing as string issue #6

fixing typing as string issue #6

Uh oh!

Conversation

yibeichan commented May 19, 2023

Uh oh!

yibeichan commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tclose commented May 23, 2023

Uh oh!

tclose commented May 23, 2023

Uh oh!

yibeichan commented May 31, 2023

Uh oh!

yibeichan commented May 31, 2023

Uh oh!

tclose commented May 31, 2023

Uh oh!

yibeichan commented May 31, 2023

Uh oh!

tclose commented May 31, 2023

Uh oh!

yibeichan commented May 31, 2023

Uh oh!

codecov-commenter commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yibeichan commented Jun 1, 2023

Uh oh!

tclose commented Jun 1, 2023

Uh oh!

tclose commented Jun 1, 2023

Uh oh!

yibeichan commented Jun 1, 2023

Uh oh!

tclose commented Jun 1, 2023

Uh oh!

yibeichan commented Jun 1, 2023

Uh oh!

tclose commented Jun 1, 2023

Uh oh!

tclose commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yibeichan commented Jun 1, 2023

Uh oh!

tclose commented Jun 1, 2023

Uh oh!

yibeichan commented Jun 1, 2023

Uh oh!

tclose commented Jul 13, 2023

Uh oh!

yibeichan commented Jul 16, 2023 via email

Uh oh!

yibeichan commented Jul 24, 2023

Uh oh!

tclose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yibeichan commented May 23, 2023 •

edited

Loading

codecov-commenter commented May 31, 2023 •

edited

Loading

tclose commented Jun 1, 2023 •

edited

Loading