-
Notifications
You must be signed in to change notification settings - Fork 181
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
New command - mrgrid #364
Comments
Yes, I think this is a great idea. Now that I started thinking about it, Maybe within the context of such a command, it might be worth to "generalize" some cropping and padding options, i.e., turning them into more unified "field of view" options (if that makes sense). |
Happy enough with all of the above - although I'm still generally of the opinion that these operations are all sufficiently similar to justify being part of a single command. I can see that there is a subtle distinction: one set of operations doesn't modify the position of the underlying object in real space or the axes used for the grid ( I'll go through my arguments again, and you can happily ignore me if you still disagree, happy to go with the majority vote on that one. First off, I would expect that all of the various modes of operation offered by In terms of actual code, I'm also thinking that merging makes sense. If On the other hand, I can see that having it all in one command would complicate So that's my 2 cents... |
I agree that some of the code paths would be the same if incorporated into On Mon, Sep 28, 2015 at 6:53 PM, J-Donald Tournier <notifications@github.com
|
Also related to #16. I think having Then again, I find the existing |
I think mrtransform should be reserved purely for transforming images wrt If anything, we should remove the ability for mrtransform to regrid without I think just because both of these commands happen to interpolate (and The only good argument I can see for including -resize into mrtransform is On Tue, Sep 29, 2015 at 10:27 PM, Robert Smith notifications@github.com
|
Fully agree with @draffelt here. It's not about what functionality happens to be used to accomplish the task, it's about the task you offer.
I mean, why do we put so much effort in coming up with good command names, if they end up not reflecting what the command does? You'll end up confusing/misleading users, and we'll get the same typical questions over and over again on the support fora. If the command name can lead the user to the right command, and from there on the help takes them further to the right options; you have software that works for the users (and provides inherent support upfront). The logic of pulling apart stuff based on what code/functionality is used, does not make a lot of sense, I find. If you reason that logic through, the concept of the apps becomes useless at some point; since about every app would end up being a very light wrapper around just one (or a small group) of back end API calls. IMO, the apps are there to make clever use of the API to accomplish higher level tasks that make sense to the workflow of actual real users. The fact that we have convenience functions such as the Filter::reslice (and about any filter, for that matter), makes that such apps can be written using minimal duplication of actual low level algorithms. Our building blocks are (convenience) API calls; the end user's building blocks are tasks. There's an important difference there; and encapsulation/abstraction allows us to make that happen. :-) |
I think if the name were changed from Sounds like Dave & Thijs's train of thought is similar, but having what I'm calling |
I'm not opposed to splitting If we really want to reduce the number of commands I think mrgrid would be On Wed, Sep 30, 2015 at 11:56 AM, Robert Smith notifications@github.com
|
Guess I'm outvoted on this one. No problem, I think we should take the consensus approach. However, since the debate is ongoing, I'll keep adding my 2 cents, see if anyone comes round to my point of view - call me stubborn... So as I've said before, I've no major objections to having these different commands in there. It's just that I don't think they're necessary, will duplicate functionality, increase the amount of code to maintain, and won't necessarily be any more obvious to users. The first few points are not necessarily a great argument if our aim is purely to make MRtrix3 the most user-friendly and awesome package out there, at all costs. The problem is that this would be fine if we had an army of developers and all the time in the world. We don't, so I think our time is better spent making sure that the few commands we do provide do their job as well and as flexibly as possible. As @draffelt kindly reminded me (the irony is not lost on me BTW - git), these commands should do one job and do it well. I think the difference in opinion here is what qualifies as 'the job'. As far as I can tell, In more detail: the reason I still favour combining all this into
So for me, these other apps basically allow you to specify point 3: the target grid. The other two are left at default values (no transformation, default interpolator, which could be nearest-neighbour if the app determines no actual resampling is necessary). I reckon if I was knowledgeable in this field and came to MRtrix3 to find you had one app to do all this, and other apps to perform the simpler operations, I might actually find this confusing: why two apps to achieve the same thing? Even the syntax wouldn't necessarily be hugely more convenient... As I see it, the main argument for having these other apps is one of discoverability: how easy is it for a new - but savvy - user to figure out how to do these things. Sure, he might go through the list of commands and notice the other apps. On the other hand, he might notice The other way of looking at this discoverability issue is that commands should be task-focused to aid discoverability, as @thijsdhollander suggested. Padding, cropping, or upsampling an image are different tasks from applying a transformation, so we should provide different apps to do these. I'm not sure I buy this approach. Often, different commands can be used to do the same task, sometimes a specific task can be performed using a command that was not explicitly designed for this task, and many times a seemingly simple task requires a number of commands. For instance, just this morning I was looking at a dataset where the slices were all out of order - It's easy to fix using So that's all I had to say. Like I said, happy to leave the final decision up to you guys, but I just thought I would clarify my position as much as possible while we're talking about it... |
So I'm leaning towards a compromise of only having one
I disagree with this point. The functionality of mrtransform only needs to be able to transform an image (see my next comment).
As I mentioned before. We could ensure there is no overlap in the functionality by enforcing that mrtransform actually transforms an image (i.e. the user must supply either -linear or -warp or both). We can then reserve regridding onto another template image grid (without a transform) for mrregrid. In all other packages I've used the transformation command requires at least one transform.
I fully agree we need to keep the code as small as possible for maintenance reasons. However, I don't think the cost is that high for this one (we have spent more time discussing this than it will take to write and maintain a
|
😁 I guess we'll have to disagree then - especially on that last point... 😉 Anyway, I've made my case, and I said I'd shut up, so I will. Up to you guys how you take it from here - ultimately you'll be using more than I will, so it's important this works for you... |
Sorry, just one one thing: if you remove the ability to use Actually, by that stage, I reckon it would be cool to add a few more options to |
You need a bigger phone to read on your train ride in :) ...as I mentioned:
So yes, mrregrid would also need a -template option to regrid (not transform!) an image onto another grid.
Yes, yes. I can see your point of view. However, I've also been working on mrtransform a bit lately and feel this is the cleanest solution. It's nice to be part of a democracy. Especially when you are on the winning team 👍 |
Hehe... Sorry, didn't see you'd already mentioned that. No worries, go for it. On that note, I notice the Nexus 4 won't be getting Android 6 Marshmallow - maybe it is time to get a bigger screen... |
OK, now I've got the bigger screen 👍, what needs to happen with this issue? Can it be closed? Has it been subsumed by other issues...? |
I guess its still open. I vote for having a single As I've said before, I think |
Yep, still fully agree with @draffelt on this as well. I'm not bothered by overlap in functionality at all, the more ways there are to achieve the same thing, the less we will be bothered by users not finding at least any of those ways. Code duplication is minimal, since most of the core functionality is nicely tucked away in the back end, where it can be maintained in just a single spot. |
To put it in another way: the little extra possible time spent on maintenance will pay off big time by a lot of extra free time not spent on user support. And happy users of course, because they won't need support in the first place (so they also gain time). You'll see, eventually, the whole planet's economy will win; just give it some time. 😉 |
…op, mrpad and mrtransform (identity). Added template and datatype options for all modes. #364
I'd like to extend First, there is more than just one line of code duplicated between
|
Sounds good to me. Only thing that jumps out at me is the overlap between the pad and crop operations - they feel like the same operation: you can crop with negative padding values, and vice-versa. But I can see the value in making the distinction in terms of discoverability, etc. Also, interesting to go back over the arguments in this debate 3 years on. Personally, I still reckon all this should go into a single |
Also related to #16, but none of the discussion around the interface for these functionalities actually appears there. |
👍 @maxpietsch I really like the
It's indeed great to look back at the arguments above... I guess I haven't changed much either though. 😛 I'm very happy with this kind of "duplication". I still think we're often overlooking most users don't think in an as abstract way as we do. Negative padding or cropping is all good an all, but if they are forced to resort to either of these to essentially do the positive version of the other, we're going to pay the price in time spent on support; but on top of that, the users also lose that time (and possibly their affinity with the software; we don't want to push them away, right?). That said, it's not a bad idea to allow for negative parameters to both of these options (cropping/padding); I can see that coming in handy for certain kinds of scripts for sure. But I still find it a bit silly to try and weed out as much "duplication" in functionality as possible. Note that a lot of the cropping functionality can "in principle" be achieved via
I've over these years, and especially in the most recent one, become more convinced of the opposite.. 😁 🤷♂️ It's a real struggle to try and explain people, particularly with an FSL background, the concept of our scanner/world/... space; some of them (otherwise very clever people!) almost stubbornly seem to keep talking in terms of "DWI space" and "T1 space" and "modality-of-choice space", even after all the required motion correction has been done, but simply because essentially the grids are not the same. The separation between @maxpietsch I do see an opportunity here to do away with another possible confusion in terminology, which is the "resizing" type of terminology: I still very often get from users that they (initially) believed this to be a transformation kind of resizing, i.e. a rigid, or even anisotropic (affine) type of scaling of the "object" in scanner space. Looking at the interface you've put together, I think we can fix this simply by a different word choice: both the resize terminology as well as all related options in both the "resize options" as well as "match options" categories could be replaced (terminology wise) by "regridding". That defines them both quite well (and essentially communicates that interpolation can happen), but also sets them jointly well apart from the cropping and padding. Everything combined (regridding / cropping / padding) still all acts on the grid, so Instead of
we could then have
So I'm essentially:
The matching options can then also move into the currently named "resizing options" category, but the category would be renamed to "regridding options". This makes sense, as e.g. in your example there's already duplication of the What do you reckon; makes sense? Or am I getting something wrong? |
Ok, I had a discussions about this with 3 people and got 3 opinions... The differences between 'transforming', 'regridding' and 'modifying the grid' might be confusing. I think the main concepts are: is there a change in location in real world coordinates and is the image interpolated? in
in
The main distinction is that real world coordinates are supposed to be changed via The downside of having The downside of integrating everything into
should yield a different output compared to
or
and so on. If we allow this, there would be quite a lot of scope for unexpected results. If we do not want to allow that, then we would need to define an order of operations. For instance transform first, regrid after -- expcept for warps where we apply regrid options to the warp (or its I'd lean towards that this is too complex for too little gain. |
I'm not surprised 😁 . That's probably the main reason to be careful about this, and make sure we define and/or separate things well and clearly.
Agreed, and I'd say: also in that order. The main distinction, I reckon, is whether a transformation happens to the object or not (i.e. whether there's the possibility to change the location in real world coordinates). That's what I'd genuinely call "a transformation", hence a responsibility/functionality The interpolation (or not) is secondary to that, I would argue. It's something that happens "when necessary", i.e. it doesn't have to happen when you transform rigidly (as that can be achieved by modifying the header only), but for most other transforms (affine/non-rigid) it'll almost always have to happen obviously. Same thing in the context of
In line with the above: fully agreed.
I personally don't see that as a downside; it makes full sense to me even. If you need to change the voxel size together with the transformation (e.g. to avoid double interpolation), you can create a grid up front as a template (and feed that to the But I also think it's, for most users, a less common scenario. And if it's a scenario they encounter, it's often due to the result needing to use the same voxel grid of an existing target image (which allows certain things in a more straight forward way down the track for some pipelines), which can simply be fed to
Yes, exactly. Your examples are just some of the many scenarios of where that becomes a nightmare to define, implement, document, support (as many users will have trouble understanding some of the intricacies of that), and ultimately also maintain (so as to not break the intricate logic of the order of those steps). And not that I personally care much about the Unix philosophy, but others here do, I thought... 😉 "small", "modular", not "large", "monolithic"
Couldn't agree more. So apart from the clear |
Ok, also implemented the posibility of regridding to template image but with changed voxel size. I am happy with the current layout of the command, let me know if you have objections before I wrap it up:
|
I think this is probably the most compelling reason to keep them separate. There's no simple, clean way to manage this that doesn't make things more difficult to reason about...
So this would be applied within Otherwise, I note you have a duplicate of the I also note other options are duplicated: Finally, it seems to me that the So the above would lead me to have this overall layout:
As mentioned above, there is a potential difference in interpretation in the Before going onto that though: I assume the command allows for multiple (non-conflicting) I'm not sure how to handle this in the ideal case, but I think something using number sequences might work well, if we use a convention like:
This then opens up the option of having negative values (not sure how keen you guys are going to be on the idea...):
At which point we've effectively merged the two Other thoughts:
Sorry, lots of random thoughts in here, just thought I'd air them here for discussion... |
The saga continues, or so people say. TL/DR see bottom. Interface: Having the out of bounds value in a general option group makes sense. However, currently it does not apply to cropping as crop does not allow negative values. Logically I'd expect common options to apply to all operations but happy to change it. I used
Yes, and I kept it that way for backwards compatibility. Having a mixture of relative and absolute axis indices might be confusing to new users but changing this behaviour might stump old users. Not sure what is better. I like the range (colon-separated) vs signed single or comma-separated tuple idea but not sure if we can handle the edge case 2:3 vs 2,3? If we go for
It's implemented in mrgrid. I was not planning to implement this in mrtransform as it is a regrid job but I guess there is no harm in duplicating code? ;)
Yes, I added the
It ignores the header transformation, as it is applied to the grid only. Hence the grouping into regrid and pad/crop arguments.
Yes, the ambiguity is the problem.
can already unambiguously be achieved (using a single interpolation) via Code: I kept the code paths for crop and pad separate as cropping relies on the subset adapter which can not handle out of bounds values. If we go for the 1,2 tuple vs colon syntax or if we just translate cropping to negative padding, we could use the padding code path for everything. That meant we'd need to modify the
How about:
|
🤣 Indeed... and it's not over - although bear in mind these are just general deliberations / internal musings / thinking aloud.
I'm not that worried about it. As long as the effect of the options is clear, the fact that they're not relevant in all contexts doesn't bother me. For the
I don't see the need to make that distinction - the operation to be performed already tells you what you expect to happen. If anything, I feel having two distinct labels attached to what is really the same concept (this is the 'reference grid') actually confuses things. Assuming we stick with the single mandatory operation argument (likely, see below), then I think we should use the same label everywhere. I don't feel all that strongly about which one we use, there are different good options here:
OK, given that this is a new command, taking over functionality previously in other commands, I don't see the rationale for trying to maintain backwards compatibility. This is basically why I came up with all these suggestions in my last post. We have an opportunity to rethink the interface, so let's not be constrained by the previous commands - but let's also make sure it remains vaguely coherent with the rest of the software.
What I was thinking in this case is that we can't use the standard number sequence handler (
Two options:
But this is only if there's appetite for moving from an interface with a single mandatory operation, to potentially multiple operations as options. It's just an idea, and as you mention, a combination of commands can already be used to achieve all of the relevant use cases with better clarity. I don't like the redundant data copy involved in that, but it's not exactly a massive problem either...
OK, got wrong end of stick. Ignore me.
OK, in which case I'm confused (happens a lot these days...). Does this mean that if I have images Conversely, if I have images I think the only way to handle this option in a predictable manner is to require the voxel grid to already match to good precision, and then apply whatever padding / cropping is necessary to make the output grid match that in the Or am I reading this wrong - again...?
OK, I think I agree sticking with the single mandatory operation argument is preferable in the interest of clarity - happy to stick with that.
I reckon it might be feasible to modify the subset adapter to support out of bounds conditions? Shouldn't™ be too hard, right?
Yes, that's what I had in mind. But I don't think the interface needs to therefore offer a single crop/pad operation. We can offer both, and simply interpret the arguments differently depending on context. So this would mean that
Cool. But would be good to make the interface accept any of these combinations without the user having to work out which combo works and which doesn't... It also makes our lives easier if we can reduce it to a single code-path. So on that basis, how about:
What do you all reckon...? |
As discussed originally in Gitlab, we decided that mrresize should be incorporated into mrtransform (#15). Even though this does not strictly transform the image, it was decided that it should be ok to have the mrresize options (-scale, -voxel, -size) as a part of the "regridding options" in mrtransform.
However, given that we also have mrcrop and mrpad, I'm wondering if it makes more sense to have a single
mrgrid
command. We could then have separate option groups for crop, pad, and resize.This would also mean that we avoid having to hide the mrresize options to mrtransform (which is already a fairly large command).
What do you think?
The text was updated successfully, but these errors were encountered: