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

add mrtrix in capsul #288

Merged
merged 3 commits into from
Aug 29, 2023
Merged

add mrtrix in capsul #288

merged 3 commits into from
Aug 29, 2023

Conversation

manuegrx
Copy link
Contributor

Hi,

I would like to add mrtrix (mrtrix3) in Capsul in order to use it in populse mia.

As I am not sure about which is the new method and which is the old one, I modified study_config, in_context and engine methods.

It seems to work fine on my side when I used mrtrix in populse (see https://github.com/populse/populse_mia/pull/313) and when I used the script the following script (with a config file: python -m capsul --config=/tmp/config_mrtrix.json /tmp/mrtrix_capsul.py /tpm/out_config.json):

from capsul.api import Process
from traits.api import File
from capsul.in_context import mrtrix
import json

class mrinfo_help(Process):
    output_config = File(output=True, desc='output file to write config',
                         allowed_extensions=['.json'])

    def requirements(self):
        return {'mrtrix': 'any'}

    def _run_process(self):
        import capsul.engine
        fconf = capsul.engine.configurations.get(
            'capsul.engine.module.mrtrix')
        if not fconf:
            raise RuntimeError('mrtrix config is not present')
        with open(self.output_config, 'w') as f:
            json.dump(fconf, f)
        mrtrix.mrtrix_check_call(['mrinfo', '-h'])

Are there any other tests I need to perform?

Moreover, regarding the test done in Capsul I have few questions:

As it is my first steps in Capsul I think it could be a greta idea if someone can check what I have done :)

Copy link
Collaborator

@sapetnioc sapetnioc left a comment

Choose a reason for hiding this comment

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

There are thing I do not know about config in Capsul v2. Especially for your question about cif value. You will have to wait a little bit to have a useful answer.

We could add tests for other modules but, as for other tests, this will only test a few elements regarding the definition of configuration values. We do not have a system for testing usage of configuration for software that may not be installed on the user system.

capsul/engine/module/mrtrix.py Outdated Show resolved Hide resolved
if config:
settings.remove_config('mrtrix', 'global',
getattr(config, cif))
settings.new_config('mrtrix', 'global', {'directory': '/there',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose that mrtrix is supposed to have a unique cif, not the same as ant. Any value would be possible here as long as it is unique among all other values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using a unique hard-coded CIF will just forbid to have several installs / versions of Mrtrix configured in Capsul, but it will probably be OK.

session.new_config(
'mrtrix', 'global',
{'directory': mrtrix_path,
cif: 'mrtrix'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I must admit here that I do not know how cif values are used. In a previous comment I supposed it was automatically generated with a random value if not set, but I do not know why there is a fixed value here. @denisri might have the answer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't remember if the CIF is automatically generated if not specified, I guess yes but I have not checked (just coming back from vacation...). It is used as a key in the config database so it just has to be unique in the config CIF values. When several configs for the same software may be configured, the CIF must differ, that's why we use values like "spm12-standalone" or "spm8" to distinguish them, but it's just a convention.

@manuegrx
Copy link
Contributor Author

Thanks for your feedback !

I will wait for Denis's advice about the cif value before to merge this branch (there's no hurry about this subject)

@manuegrx
Copy link
Contributor Author

Moreover some of the GitHub automatic test are failing, is it normal ?

@denisri
Copy link
Collaborator

denisri commented Aug 29, 2023

Thanks for your feedback !

I will wait for Denis's advice about the cif value before to merge this branch (there's no hurry about this subject)

I haven't read the code thoroughly but it seems OK for me.

@denisri
Copy link
Collaborator

denisri commented Aug 29, 2023

Moreover some of the GitHub automatic test are failing, is it normal ?

codespell has many false positive that I don't know how to disable. I'm not sure if we should use it at all, it's sometimes useful but makes checks systematically fail in many projects.
For Macos tests, I have no idea...

@manuegrx manuegrx merged commit 27a82d4 into master Aug 29, 2023
1 of 11 checks passed
@manuegrx manuegrx deleted the add_mrtrix branch August 29, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants