Tag 3.0 RC2 #1052
Tag 3.0 RC2 #1052
Conversation
tractography.load (loads a track file) tractography.slab (slab thickness) tractography.tsf (uses a tsf file for track node colours) WARNING: tsf optional arguments do not appear to actually be optional due to pre-existing code enforcing 'optional' args (bug?).
… LeeReidCSIRO-master
Error message now to stderr, not as window
… command line commands) to call a single method
This header should no longer be included directly, since class MR::vector (defined in core/types.h) should always be used.
As discussed in #996. Also some other minor cosmetic changes.
See discussion on #996
This is useful in non-DTI contexts (e.g. for structure tensor analysis).
…issue images with additional bias field correction
* Also minor clean-up
new mrdegibbs command
Python fsl.findimage() function
|
Ok, everything's ready and all tests check out. The original scope and more is done. I note a few thumbs up in the staff category discussion. As mentioned there, unless anyone voices concerns, the new RC2 can become a reality tomorrow. 😌 I'll keep an eye on here and the staff category discussion, and unless concerns are voiced, make things happen somewhere AM AEST... That should give the other time zones some final (day) time to respond if needed. |
|
@thijsdhollander In In the case of a zeroth order polynomial, the "norm" field equals the estimated scale factor which is also stored in the image header under the entry I might have missed it buried in the discussions here but what is the meaning of Edit: Just saw your comment in the code which describes the On the other hand, a global scale factor similar to Any thoughts? |
|
Happy for release - as long as @maxpietsch is happy, of course... 😉 40°C over here in Greece - sweltering... If it all goes south, I'm off-grid. 😁 |
|
Don't let my comment deter you from merging! |
|
@maxpietsch : I'll merge somewhere this morning, but I'll get back to your comments above too. They're indeed valid points, or rather "choices", that can be made when providing those outputs, and the overall rationale of the new EDIT: so to clarify, I'll post a bit of a longer explanation later on. Eventually, I should probably include some of it in the userdocs somewhere too. |
This should address issue #1081
Handle shell selection when using msmt_csd
I can overcome this now because I wasn't the one creating the pull request, but we may want to think about this setting/behaviour in the future. After looking this up, it looks like this can be set: i.e. if new commits undo any reviews. This makes sense in theory, but in practice may be annoying for our work flow if e.g. any last minute bug fixes are done (as is essentially the case here now). |
|
Something like |
|
Good to see this is now released! Good effort everyone. Just one thing: I note the tag is not signed...? At least it's not showing up as verified on the releases page. I'd like us to stick to signed releases to give the software a clear seal of approval. @thijsdhollander, any chance you can rectify? Procedure is explained here and other places. I expect you'll need to delete the existing tag and create a new signed version, which you can do on your local repo, but you won't have the rights to force-push the updated tag to the main repo from your account. Simplest at this point would be for you to push the correct tag to your own fork of the repo, and let me know where to fetch it from. I can then fetch your repo and force-push it from my side. How does that sound? Note there's no great rush to do this, take your time... |
Well, the whole point of enforcing these reviews is to ensure all changes that get pushed to As far as our workflow is concerned, I don't see that this should be a problem in any way. From this point on, we should not be pushing anything to In this case, all this would have done is delay the release a touch while someone else provides an up to date review - which you were able to do yourself here anyway. So I don't see the need to change anything - hope you all agree...? |
|
Ah; should probably have rebased #1083 to Agree with keeping the stale review settings as-is. It's possible to create a PR but not yet request a review if there's a good chance of minor fixes before merge. Also, splitting development work in |
Fair enough; happy with it. Let's see how it plays out in practice.
Aha, interesting. I hadn't spotted this (functionality) yet, so was unaware of it all. Ok, I've checked out a bunch of documentation on it and set up my GPG key to get it to work. I've forked the repo here: https://github.com/thijsdhollander/mrtrix3 ...and pulled it locally, removed the original (unsigned) tag, created a new (signed) tag to the exact same commit (the merge commit), and force pushed again to the forked repo (apparently had to force push because a / the tag with the same name already existed). This all looks ok now: https://github.com/thijsdhollander/mrtrix3/releases/tag/3.0_RC2 @jdtournier, I suppose this gives you the means to fetch my fork and force push to the main one? I can make it into a "release" then again as well. Going through the process and learning about the steps involved, I did start to wonder how I should have done it it practice originally though: wouldn't I have run into the same problem, i.e. that I can't push directly to our |
|
Ok, forget most of my post above: I've figured out the answer to my last question and I fixed the signed tag straight on the main MRtrix3 repo's So I found out that branch protection doesn't interfere with pushing a tag. I did think that force pushing a tag (deletion and addition with the same tag name) would almost certainly be stopped by branch protection... but guess what: it isn't. After I'd done my tag business on my local copy of the main MRtrix3 repo's https://github.com/MRtrix3/mrtrix3/releases/tag/3.0_RC2 Weird stuff. 😮 |
😲 That's unexpected... I guess it makes sense since a tag is not a branch, and there's not tag protection option, but since tags show up as releases, it's a little scary... In any case, I guess at least that problem is now solved... 👍 |
Yes, I didn't spot that either... Oh well, not to worry. I'm not sure how to play this to be honest. Maybe the simplest is to do as you did: push overt bug fixes (whether critical or not) to I think we'll need to codify all this and document it somewhere at some point... Maybe via doxygen, so it can show up in the online dev docs...? With a link to it in the README? |
Sleeping a night over it, this makes full sense to me as well now. Tags have essentially nothing to do with any particular branch. You can just tag any commit, or even any object (doesn't even have to be a commit apparently, but GitHub will only show tags to commits on its website). But I agree it's a bit scary (though we can't really do anything about that). Note furthermore that I didn't have to re-create the "release" thing online ("releases" are basically further annotation and possibly attached binaries etc... that only live/exist on GitHub, not in the actual repository; and are linked to a tag)! You can see this by looking at the date/time of the release versus the tag: the tag (since I had to re-do it) has a time stamp that is more recent than the release (which retains its original time stamp). This behaviour is what I find the most scary in a way: you could make a tag and release, then add compiled binaries or other files to it via GitHub, yet subsequently someone (of us) could change the tag with the same name to point to anything else, and that would inherently change where that "release" points to (even though the binaries or other attachments are completely independent of course). So you can in principle end up with an inconsistent state. Note even furthermore that I could've changed the tag even more easily without deleting the former, by using the So well, lesson here is probably that we'll simply have to be careful. There's no GitHub facilities that I'm aware of that offer any further protection on this front... Finally, @jdtournier @Lestropie @maxpietsch @rtabbara (since you all had open pull requests, or branches that were rebased to |
Constructing a discrete pull request for the new tag. Not just to create the button, but also to collate the changes coming in & going out. I think we'll all agree that time is a priority on this one, so only bits that really need to go into the tag should be listed; need to avoid scope creep.
Incoming:
mtlognorm #1036 (
mtbinreplacement): Main reason for tag.fixelcfestats: New option -mask #1030 (
fixelcfestats -mask): Already merged in.Dwipreproc & PhaseEncoding handling fixes #1047 (
dwipreprocfixes for acquisitions that include repeating all DWIs): Basically done, but I might think about & tweak a couple of things regarding handling of the header entries. Realistically doesn't need to go with a tag since it's an outright bug fix and shouldn't influence outcomes for use cases that were otherwise working, but there's no harm in going along with.Dwipreproc alignment #1051 (
dwipreprochandling alignment betweentopupoutput andeddy): Looks like I might have something working, but ideally needs a bit of testing. Would be reasonably valuable if it could be validated in time; distortion correction errors could be substantial if there's a fair bit of inter-protocol motion, so it's an important one to fix.Other pull requests remain as merging into
dev.