Meshfix interface inverting command parameters

[MartinGrignard]

Summary

Hi,
While using the MeshFix interface, I received this error:

RuntimeError: Command:
meshfix /path/to/file.off -a 2.000000 0.200000 --fineTuneIn  4 --shells 2 -o file_fixed.off -q
Standard output:
Fixing asin tolerance to 3.491368e-02
Standard error:

ERROR- number of substeps must be > 0.
Return code: 255

Actual behavior

As shown previously, the interface generated command leading to this error is:

meshfix /path/to/file.off -a 2.000000 0.200000 --fineTuneIn  4 --shells 2 -o file_fixed.off -q

And the error is pretty clear, the --fineTuneIn argument requires a distance and a substeps number. Those two parameters are actualy given in the code:

finetune_wm = pe.Node(interface=mf.MeshFix(), name='finetune_wm')
finetune_wm.inputs.quiet_mode = True
finetune_wm.inputs.epsilon_angle = 2
finetune_wm.inputs.number_of_biggest_shells = 2
finetune_wm.inputs.finetuning_substeps = 4       # Set the substeps number
finetune_wm.inputs.finetuning_distance = 0.2     # Set the distance
finetune_wm.inputs.finetuning_inwards = True     # Set --fineTuneIn

And these parameters are also in the command but in a wrong order:

0.200000 --fineTuneIn  4

Expected behavior

The correct order should be:

--fineTuneIn 0.200000  4

As writen in the doc:

--fineTuneIn <d> <n> Used to fine-tune the minimal distance between surfaces
A minimal distance d is ensured, and reached in n substeps

And then the command should be:

meshfix /path/to/file.off -a 2.000000 --fineTuneIn 0.200000  4 --shells 2 -o file_fixed.off -q

[mgxd]

Hi Martin - this is probably the result of changes from when the MeshFix interface was originally created (sometime in v1.x).

If the API is significantly different, we may want to create a new interface MeshFix2 to cover v2.x. Would you be interesting in working on this and submitting a Pull Request? If so, feel free to open up a Work In Progress PR and we can provide assistance.

[MartinGrignard]

@mgxd I actually installed v1.2 as this is the compatible one. This is why I created an issue (now that I figured out that the v2 is not compatible).
Creating a MeshFix2 interface could be interesting. I might do this in the next months (not now as I am deeply focused on Forward).

I'm still not familiar with traited input. I looked at the MeshFix interface and it looks strange to me to have things like this:

uniform_remeshing_steps = traits.Int(
        argstr='-u %d',
        requires=['uniform_remeshing_vertices'],
        desc="Number of steps for uniform remeshing of the whole mesh")

But also:

finetuning_inwards = traits.Bool(
    argstr='--fineTuneIn ',
    requires=['finetuning_distance', 'finetuning_substeps'])
finetuning_distance = traits.Float(
    argstr='%f',
    requires=['finetuning_substeps'],
    desc="Used to fine-tune the minimal distance between surfaces."
    "A minimal distance d is ensured, and reached in n substeps. When using the surfaces for subsequent volume meshing by gmsh, this step prevent too flat tetrahedra2)"
)
finetuning_substeps = traits.Int(
    argstr='%d',
    requires=['finetuning_distance'],
    desc="Used to fine-tune the minimal distance between surfaces."
    "A minimal distance d is ensured, and reached in n substeps. When using the surfaces for subsequent volume meshing by gmsh, this step prevent too flat tetrahedra2)"
)

As the --fineTuneIn and --fineTuneOut require the distance and substeps fields, isn't it possible to write something like:

finetuning_inwards = traits.SOMETHING(
    argstr='--fineTuneIn %f %d'
)

Or is there a way to modify the order in which the parameters are written?

[effigies]

finetuning_inwards = traits.SOMETHING(
    argstr='--fineTuneIn %f %d'
)

Yes, you should be able to do:

traits.Tuple((traits.Float, traits.Int), ...)

Or is there a way to modify the order in which the parameters are written?

I think this is the better solution, to avoid changing the interface. Use the position keyword. See the developer docs for its use. (Not every attribute needs to be given explicit position.)

[MartinGrignard]

Thanks @effigies, the position attribute did the work:

    finetuning_inwards = traits.Bool(
        argstr='--fineTuneIn ',
        requires=['finetuning_distance', 'finetuning_substeps'],
        position=-3
    )
    finetuning_outwards = traits.Bool(
        argstr='--fineTuneOut ',
        requires=['finetuning_distance', 'finetuning_substeps'],
        xor=['finetuning_inwards'],
        desc='Similar to finetuning_inwards, but ensures minimal distance in the other direction',
        position=-3
    )
    finetuning_distance = traits.Float(
        argstr='%f',
        requires=['finetuning_substeps'],
        desc="Used to fine-tune the minimal distance between surfaces."
        "A minimal distance d is ensured, and reached in n substeps. When using the surfaces for subsequent volume meshing by gmsh, this step prevent too flat tetrahedra2)",
        position=-2
    )
    finetuning_substeps = traits.Int(
        argstr='%d',
        requires=['finetuning_distance'],
        desc="Used to fine-tune the minimal distance between surfaces."
        "A minimal distance d is ensured, and reached in n substeps. When using the surfaces for subsequent volume meshing by gmsh, this step prevent too flat tetrahedra2)",
        position=-1
    )

It looks more like a temporary fix than like a proper solution but it allows me to keep working.
I'm currently working on my own branch of Nipype in order to use custom Gmsh and GetDP interfaces so I suppose it is not possible to push these modifications.

Once again a great thanks for your help!

[effigies]

@MartinGrignard Would you like to submit a PR with those changes? I'm sure any other users of the interface would appreciate it.

[MartinGrignard]

@effigies I'll submit a pull request when I'll be sure that every MeshFix related error in my project will be fixed because for now. When I'll be ready to do so, I suppose it would be easier to create a branch with only MeshFix modifications? Or I could also propose Gmsh and GetDP interfaces at the same time?

[effigies]

It will generally be easier to verify small fixes than review a new interface, so if you do separate PRs, some fixes could go in quickly even if it other changes will take longer to review.

[effigies]

I'm going to reopen this, by the way. Just add "Fixes #2770" to the text when you do submit a PR. It will help keep conversations connected.

[MartinGrignard]

Ok I'll make sure to do this properly.