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

New command: mrhistmatch #671

Closed
Lestropie opened this issue Jun 17, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@Lestropie
Copy link
Member

commented Jun 17, 2016

Apart from porting the old code over to the latest syntax, should also move algorithm to header files somewhere so that it can be used from within the registration framework.

@Lestropie Lestropie self-assigned this Jun 17, 2016

@Lestropie Lestropie added this to the Stanford Coding Sprint milestone Jul 15, 2016

@Lestropie Lestropie added the question label Jul 22, 2016

@Lestropie

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

Was trying to homogenize histogram functionality across code as I did this; didn't want 3 separate blocks of code for generating histograms. In mrstats there's some weird functionalities in there that I presume were useful to someone at some time, but really make the code quite difficult to work with, especially when trying to add in things like #71 - end up with factorial expansion of code branching as a function of option count.

For instance: the mrstats -position option outputs the voxel locations of all tested voxels to a text file. This I would imagine would be all but useless if statistics are calculated either across the entire image, or based on the -voxels option (where each voxel to process must be provided as a separate integer triplet at the command line); meaning that it's only useful if using the -mask option. Therefore I don't think this really needs to be / belongs in mrstats, since it's not computing statistics per se, and is only applicable in 1 out of 3 mrstats use cases; it was probably just convenient to put it there at the time. Could this go into its own command; e.g. mask2pos, maskvoxellist, variations thereof?

There are other possible changes also. For example, statistic calculation and histogram generation are essentially independent; sure you save time by doing both at once, but that's not likely to have an influence really. Separating them might make the functionality a little cleaner; whether users would be more or less likely to discover the capability, I don't know... mrhist / fixelhist for generating histograms only?

Another feature I'm pessimistic about now is mrthreshold -histogram. I'm guessing this is left over from before the Ridgway 2009 method for threshold determination was implemented? Trouble is that here I've implemented a better heuristic for automatically determining the number of histogram bins when it's not set explicitly, but I'm pretty sure this will be too fine for this threshold determination method (gradient ascent then descent from the bottom end of the histogram). Could change to a hard-coded number of bins just for mrthreshold, but I'm not sure this method itself is something we'd actually advocate using. Are we better off just removing this functionality?

@thijsdhollander

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

Could change to a hard-coded number of bins just for mrthreshold, but I'm not sure this method itself is something we'd actually advocate using. Are we better off just removing this functionality?

As long as mrthreshold -histogram isn't used in any existing scripts, I'm happy to get rid of it. As long as the Ridgway 2009 method remains untouched and intact; and it'd be good to get #738 fixed as well in that context. :-)

@jdtournier

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Yep, agree with everything here. A lot of this stuff is historical, dating back to before even the release of version 0.2... I don't know of anything that relies on any of this functionality currently, simplest might indeed be to just remove the cruft.

Lestropie added a commit that referenced this issue Jul 25, 2016

Further changes to stats and histogram code
Following from discussion in #671.
- Remove histogram generation functionality from mrstats and fixelstats; these are instead handled using new commands mrhistogram and fixelhistogram.
- Removed some historical capabilities from mrstats that were limiting code expansion.
- New option for mrstats to generate statistics across all volumes, rather than the default (separate statistics for each volume).
- Histograms should now detect when the input data consists of integers, and set the bin width to be an integer accordingly.
Closes #71.
@Lestropie

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2016

OK, let's see if people think that's appropriately un-crufted...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.