{"action":"created","issue":{"url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/issues/1621","repository_url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox","labels_url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/issues/1621/labels{/name}","comments_url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/issues/1621/comments","events_url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/issues/1621/events","html_url":"https://github.com/neuropoly/spinalcordtoolbox/issues/1621","id":299960808,"number":1621,"title":"Proposal/discussion: proper Python API for SCT (vs. subprocesses or calling module.main(args=[...]))","user":{"login":"zougloub","id":998040,"avatar_url":"https://avatars1.githubusercontent.com/u/998040?v=4","gravatar_id":"","url":"https://api.github.com/users/zougloub","html_url":"https://github.com/zougloub","followers_url":"https://api.github.com/users/zougloub/followers","following_url":"https://api.github.com/users/zougloub/following{/other_user}","gists_url":"https://api.github.com/users/zougloub/gists{/gist_id}","starred_url":"https://api.github.com/users/zougloub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/zougloub/subscriptions","organizations_url":"https://api.github.com/users/zougloub/orgs","repos_url":"https://api.github.com/users/zougloub/repos","events_url":"https://api.github.com/users/zougloub/events{/privacy}","received_events_url":"https://api.github.com/users/zougloub/received_events","type":"User","site_admin":false},"labels":[{"id":625973089,"url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/labels/card:DESIGN_DISCUSSION","name":"card:DESIGN_DISCUSSION","color":"f9d0c4","default":false},{"id":137139873,"url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/labels/feature","name":"feature","color":"5319e7","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-02-24T18:15:33Z","updated_at":"2018-02-27T06:57:32Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"Currently SCT is mostly implemented as a command-line API and even the internal calls are working this way.\r\n\r\nThe code is making heavy use (318 calls) of subprocess calls:\r\n```\r\n     53 scripts/sct_make_ground_truth.py\r\n     48 scripts/sct_register_to_template.py\r\n     40 scripts/sct_register_graymatter.py\r\n     20 scripts/sct_register_multimodal.py\r\n     16 scripts/sct_label_vertebrae.py\r\n     13 scripts/sct_straighten_spinalcord.py\r\n     10 scripts/sct_dmri_eddy_correct.py\r\n      9 scripts/sct_create_mask.py\r\n      9 scripts/sct_convert.py\r\n      8 scripts/msct_smooth.py\r\n      7 scripts/sct_process_segmentation.py\r\n      7 scripts/sct_image.py\r\n      7 scripts/sct_check_dependencies.py\r\n      6 scripts/isct_test_ants.py\r\n      5 scripts/sct_dmri_separate_b0_and_dwi.py\r\n      5 scripts/sct_dmri_moco.py\r\n      5 scripts/msct_moco.py\r\n      4 scripts/sct_smooth_spinalcord.py\r\n      4 scripts/sct_get_centerline.py\r\n      4 scripts/sct_compute_hausdorff_distance.py\r\n      4 scripts/sct_apply_transfo.py\r\n      4 scripts/msct_register.py\r\n      4 scripts/isct_convert_binary_to_trilinear.py\r\n      3 scripts/sct_fmri_moco.py\r\n      3 scripts/sct_flatten_sagittal.py\r\n      3 scripts/sct_dice_coefficient.py\r\n      2 scripts/sct_testing.py\r\n      2 scripts/sct_detect_pmj.py\r\n      2 scripts/sct_crop_image.py\r\n      2 scripts/isct_warpmovie_generator.py\r\n      1 scripts/sct_warp_template.py\r\n      1 scripts/sct_viewer.py\r\n      1 scripts/sct_utils.py\r\n      1 scripts/sct_propseg.py\r\n      1 scripts/sct_pipeline.py\r\n      1 scripts/sct_deepseg_sc.py\r\n      1 scripts/sct_concat_transfo.py\r\n      1 scripts/sct_compute_mtr.py\r\n      1 scripts/msct_image.py\r\n```\r\n\r\nA WIP is to create a `main()` function in modules, which is good practice for avoiding `__main__`.\r\nThis approach gets rid of one or two subprocess calls (one if `sct.run()` is called with a list (now in #1618), two if it's with a string) and also reduces module loading time, but the syntax is about the same, and we're still reading/writing a ton of temporary files.\r\n\r\nA Python API going beyond this `main()` would be very useful:\r\n\r\n- Having the processing code in `spinalcordtoolbox` folder/namespace\r\n- Several functions per module\r\n- By using placeholders, functions can take/save file paths, or deal with in-memory contents\r\n- Pass optional hierarchical logger as argument  \r\n  That would allow to have a better graphical understanding of the program flow\r\n- The scripting entry points would be made faster to call since dependencies would be imported on actual execution of the scripting entry point  \r\n  Today calling a program for help can take a while...\r\n```\r\n$ time sct_apply_transfo \r\n[...]\r\nreal    0m1.811s\r\nuser    0m1.676s\r\nsys     0m0.131s\r\n```\r\n\r\nTo proceed with the implementation:\r\n\r\n- Start by the most used internal calls (eg. 30 calls to `sct_image -setorient RPI ...`)\r\n- Be really careful about the external API, which will have to last\r\n\r\nSome proposals:\r\n- `import spinalcordtoolbox as sct`  \r\nKind of obvious that we'd want the users to do that. And the internal code would probably use `sctu` for utils.\r\n- `sct.image.reorient(src, dst=None, orientation=\"RPI\")`  \r\n  When `dst is None`, the output data is saved into src. That's kind of like the OpenCV python API.\r\n  Parameters are given meaningful parameters and defaults according to the most common use cases.\r\n- `sct.maths.binarize(src, dst=None, threshold=None, method=\"fixed\")`  \r\n  Function names to make a little more sense.  \r\n  For binarization/thresholding/etc, could try to stay close to existing APIs, and could also put methods like otsu/otsu_adap/otsu_median to get the threshold automagically.\r\n- `sct.image.crop(src, dst=None, spec=(2, 1, -1))`  \r\n  `sct.image.crop(src, dst=None, spec=[(0, 1, -1), (2, 1, -1)])`  \r\n  Could use a cropping specification: cropping could happen on different dimensions at once.\r\n- `sct.transfo.warp(src, dst=None, warping_field=None, interpolation=\"nn\")`\r\n- `sct.transfo.concatenate(src=None, dst=None, transforms=list())`  \r\n- `sct.gm.segment(src, dst=None, method=\"fast\", method_kw=None)` \r\n  In relation with the discussion about pluggable methods to do the same thing...  \r\n  `sct.gm.segment_deepseg(src, dst=None, ...)` \r\n  Which users could also call.\r\n  Note that `spinalcordtoolbox.deepseg_gm.deepseg_gm.segment_file()` call is probably not consistent with anything we could come up with.\r\n- Supporting long file names or filenames with spaces when using the Python API. There's no reason why it wouldn't work.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/issues/comments/368766799","html_url":"https://github.com/neuropoly/spinalcordtoolbox/issues/1621#issuecomment-368766799","issue_url":"https://api.github.com/repos/neuropoly/spinalcordtoolbox/issues/1621","id":368766799,"user":{"login":"peristeri","id":840040,"avatar_url":"https://avatars1.githubusercontent.com/u/840040?v=4","gravatar_id":"","url":"https://api.github.com/users/peristeri","html_url":"https://github.com/peristeri","followers_url":"https://api.github.com/users/peristeri/followers","following_url":"https://api.github.com/users/peristeri/following{/other_user}","gists_url":"https://api.github.com/users/peristeri/gists{/gist_id}","starred_url":"https://api.github.com/users/peristeri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/peristeri/subscriptions","organizations_url":"https://api.github.com/users/peristeri/orgs","repos_url":"https://api.github.com/users/peristeri/repos","events_url":"https://api.github.com/users/peristeri/events{/privacy}","received_events_url":"https://api.github.com/users/peristeri/received_events","type":"User","site_admin":false},"created_at":"2018-02-27T06:57:32Z","updated_at":"2018-02-27T06:57:32Z","author_association":"CONTRIBUTOR","body":"Note: The initial attempt to replace all `sct.run` functions with API calls was abandon due to the cyclical dependencies that caused exceptions. Much of the details are forgotten, you can examine the `devel` branch to see where things have gone wrong.\r\n\r\n@perone \r\nPoint 1. I agree with the importance of improving the memory consumption of Image. But it is outside the scope of this issue. \r\nPoint 2, you can take it one step further and introduce a FileManagement class that deals efficient use of memory of all images. Again, this is out of scope, but this a good step towards that direction. \r\nPoint 3, I agree, and it should be done strategically."}}
{"actor":{"display_login":"peristeri"}}