[ENH] Generate BIDS Derivatives-compatible outputs

[tsalo]

Closes #146 and closes #649.

Changes proposed in this pull request:

• Rename outputs to be as BIDS Derivatives-compatible as possible.
• Reorganize metric and decomposition files. Metrics are now stored in TSVs, with associated JSONs describing the metrics, and decomposition files should match BEP012 convention.

[tsalo]

I'm still wary of global variables, so I was thinking that a new OutputGenerator class could do the same work by storing ref_img, out_dir, prefix, convention, and the loaded config dictionaries (stored as jsons, as currently implemented) as attributes. Since we're already passing around ref_img, this would keep the number of arguments to most of the functions the same as they are now. I've started mocking something up that I can proposed in a PR to this branch, but I thought I'd bring up the idea here first.

EDIT: Here is the current, not-yet-functional idea.

[jbteves]

That seems fine to me. Then if we want to add outputs (for example, writing metrics to .csv formats) it's pretty straightforward to add it to that class. I'd debated a similar solution but historically we've avoided object-oriented solutions to problems so I thought my procedural implementation would be more suitable.

I'm not particularly scared of global variables in this context because they're truly global, though, and then this output generator will also effectively be a global variable.

I'd be curious to hear what @emdupre and @handwerkerd think.

[handwerkerd]

I just skimmed @tsalo's OutputGenerator and it looks conceptually fine. I'm also fine with the currently proposed globals. The one advantage I see with OutputGenerator is, if there is a creep towards creating more globally customizable options (i.e. svg & png options), this will keep these options in a more coherent and structured place.

[emdupre]

Just a +1 from me for the class-based approach, here.

You're definitely right @jbteves that we've generally avoided classes, and I at least can confess to being wary of bringing them in prematurely (having seen how quickly they can grow in complexity). But in this case I think a class has the dual benefit of being a bit easier to conceptually track in the long run (!), and putting us on a firm footing away from global-scope-creep.

[jbteves]

I'm on board with moving to a class-based approach if everyone else is, then. It's certainly more elegant than what I merged into here 🙂. That said, I wonder if we should open an issue to discuss/specify the full interface since it would be a paradigm shift. I also wonder if we should go ahead and merge this as something of a temporary fix while we hash out the details? The output generator class opens up quite a few possibilities for how we handle output, and this set of changes is relatively contained (though I admit I introduced quite a bit of scope creep already 😬): it focuses exclusively on images and metric data/metadata. Right now, the report and figure generation is completely out of the scope of this PR even with my changes, but a full OutputGenerator would handle those as well, I assume. It would also presumably fill in the dataset description, which requires more file-tracking. It could be a very large set of changes at the end. Thoughts?

[handwerkerd]

I'm fine if someone opens an issue to expand the discussion on an overall structure for I/O and UI. Even if having a coherent plan for UI & I/O may be a bit premature, having a place for the discussion might guide some decisions. That said, I think we have a fairly clear path forward for this PR. I wouldn't want that discussion to hold up finishing and merging this PR.

[tsalo]

I was able to make progress on #711 this past weekend, but I wasn't able to get it ready for review. As such, I'm happy to move forward with this PR as-is (after we address #718 and #721 and release 0.0.10, of course), and then merge #711 into main instead. @jbteves if you have the time and interest to tackle #711, I would appreciate the help. If not, then no pressure!

[jbteves]

Sorry, to clarify, the order you'd like is:

1. Merge [DOC] Add warning about not using release-drafter releases to developer instructions #718, [MAINT] Drop 3.5 support and begin 3.8 and 3.9 support #721
2. Release 0.0.10
3. Merge Use class for managing outputs #711 into this
4. Merge this to main

?

RE: working on #711, sure, I can take that over for you this week. I'm taking a short week though so I may not report further on progress until early next week instead.

[tsalo]

If I had gotten #711 ready in time, then that's the order I would have preferred. Since it's not ready, though, I'm happy with:

1. Merge [DOC] Add warning about not using release-drafter releases to developer instructions #718, [MAINT] Drop 3.5 support and begin 3.8 and 3.9 support #721
2. Release 0.0.10
3. Merge this into main
4. Merge Use class for managing outputs #711 into main

It really depends on how fast we can get the necessary reviews for #718 and #721, and when we'll be able to get #711 ready (which may depend on how much time you have for it).

[jbteves]

Alright, that makes sense. Thanks for the clarification. Could we mark this as ready for review, review it, and then plan on merging in #711 if it's finished by next Friday, or else just going ahead and merging this, and then retargeting #711 to main? That seems like a pretty decent timeline for me.

[handwerkerd]

Is constants.py no longer used? I think it's a vestige from before we switched to the class structure.

[jbteves]

@tsalo just want your confirmation before I merge this behemoth.

[tsalo]

Thanks for the heads up. Since we just made a release, I don't think we're under any pressure to release anything immediately after merging this, so I'm happy to see this merged today, and then we can triple check everything before 0.0.11 (or 0.1.0, depending on how we feel).