rordenlab / dcm2niix

dcm2nii DICOM to NIfTI converter: compiled versions available from NITRC
https://www.nitrc.org/plugins/mwiki/index.php/dcm2nii:MainPage
Other
872 stars 229 forks source link

Tag and push commit for 25 Nov. version #66

Closed ghisvail closed 7 years ago

ghisvail commented 7 years ago

Could you tag and push the following commit as v1.0.20161125 please?

neurolabusc commented 7 years ago

I suggest we test the current build a bit, as I noted here https://www.nitrc.org/forum/message.php?msg_id=19507 the latest version includes a kludge to handle images that include icons where the icon does not match the transfer syntax of the main image. By handling a clear (but potentially common) violation of the DICOM standard, it is possible we may have unintended consequences for legitimate DICOM images. I think we want developers to test this a bit before we deploy this to the wild.

ghisvail commented 7 years ago

FYI, I am currently using v1.0.20161101.

ghisvail commented 7 years ago

It was recently brought to my attention by @yarikoptic that the versioning scheme of dcm2niix had changed a few times (#28). This leads us to an uncomfortable situation whereby the NeuroDebian packages (which follow the old YYYYMMDD scheme) and Debian packages (currently following the 1.0.YYYYMMDD scheme) have incompatible upgrade paths, since upgrades are triggered by version number sorting.

Your last post in #28 reads like a more complicated versioning scheme than YYYYMMDD would not be required, which was followed by an ack from @yarikoptic. I just wanted to know what made you change from YYYYMMDD to the current versioning scheme and whether you would see an inconvenience in returning to using the former from future releases thereon.

My personal opinion is that if dcm2niix were a library, there would be plenty of room for discussion regarding which version scheme is best. For a single application however, a simple chronological versioning scheme such as YYYYMMDD is probably enough. Again, just my opinion.

neurolabusc commented 7 years ago

I had originally used the YYYYMMDD scheme, but was encouraged to move to a semver scheme, as noted earlier. I think this is a mature release, and do not see many changes other than keeping up to date with the vendor's reinterpretation of DICOM. I am agnostic about this, but think we should be consistent, so to be consistent with the last release I suggest 1.0.YYYYMMDD going forward.

ghisvail commented 7 years ago

Quoting the last post of the thread you linked:

As long as MRIcroGL has a stable (ascending) versioning scheme as part of the filename over time it's ok. Otherwise package managers have a hard time distinguising new versions over old ones.

That's exactly the situation we are in now.

ghisvail commented 7 years ago

Hi Chris,

FYI, the deadline for updating a package to a new release for the next stable version of Debian is on January 25th (tomorrow). Shall I take the current snapshot of master as of today, or are you happy keeping the old v1.0.20161101?

Cheers, Ghis

neurolabusc commented 7 years ago

Waiting for @chrisfilo to respond - he is the maintainer for the BIDS component. My recent fix for (#84)[https://github.com/rordenlab/dcm2niix/issues/84] introduces a new BIDS item. I do thing the software is overdue for a new release.

chrisgorgo commented 7 years ago

Apologies for the late reply.

The JSON sidecar can be amended with any custom key/value pair as long as the key is not previously specified in the spec. This means that addition of the "dcm2niixVersion": “v1.0.20170101” key/value is perfectly fine.

However, because the key name is so specific to dcm2niix it's unlikely this would be included in the new revision of BIDS as part of the standard. Your previous suggestion: “DicomConversion": [“dcm2niix", "v1.0.20170101"] is more software agnostic. To keep it more in line with other key/values derived from DICOM ontology and make it more agnostic to the original format of the data (after all dcm2niix also works with other types than DICOM) I would propose splitting this into two explicit keys:

"ConversionSoftwareName": "dcm2niix",
"ConversionSoftwareVersion": "v1.0.20170101"

One more clarification on commas. The fact that the last element in a list cannot have a trailing comma is part of the JSON specification. Changing this would break a lot of existing code relying on the plethora of JSON libraries available for many languages.

neurolabusc commented 7 years ago

I have generated a new release of dcm2niix. I would greatly appreciate if people could try out the latest version in the MRIcroGL release - as MRIcroGL includes not only dcm2niix but a GUI for using dcm2niix (the 'Import' menu). Unless there are any issues reported I will release the compiled code on NITRC next week.

jonclayden commented 7 years ago

I've done a quick test with a few of my datasets (on macOS), and the conversion seems to work fine through MRIcroGL.

yarikoptic commented 7 years ago

@neurolabusc 1cent: as previously suggested (https://github.com/rordenlab/dcm2niix/search?q=annotated&type=Issues&utf8=%E2%9C%93) better to use annotated tags for the releases, which wasn't the case

$> git describe  v1.0.20170130                               
20160606-174-gcf4a5e1

$> git show v1.0.20170130
commit cf4a5e1236bbf39540bc110cb819ceeec001b661
Author: Chris Rorden <rorden@mailbox.sc.edu>
Date:   Mon Jan 30 08:51:09 2017 -0500

    new release

diff --git a/README.md b/README.md
index 70653cf..e996ef4 100644
--- a/README.md
+++ b/README.md
@@ -11,7 +11,7 @@ This software is open source. The bulk of the code is covered by the BSD license

 ## Versions

-1-Jan-2017
+30-Jan-2017
  - Handle 3D Philips DICOM files where images are not stored in a spatially contiguous order.
  - Handle DICOM violations where icon is uncompressed but image data is compressed.
  - Best guess matrix for 2D slices (similar to dcm2nii, SPM and MRIconvert).
neurolabusc commented 7 years ago

@yarikoptic - my fault entirely. I am not sure how to do that, I built the release using the web-based github interface, and did not see any options for this. Is there a concise github documentation or can you provide a few lines regarding how to do this. I do like how the online GUI editor allows me to attach precompiled exectuables.

ghisvail commented 7 years ago

On Mon, 2017-01-30 at 09:56 -0800, Chris Rorden wrote:

@yarikoptic - my fault entirely. I am not sure how to do that, I built the release using the web-based github interface, and did not see any options for this. Is there a concise github documentation or can you provide a few lines regarding how to do this. I do like how the online GUI editor allows me to attach precompiled exectuables. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

You can run git tag -a $VERSION -m $MESSAGE on the commit where you want to make the release. Add -s if you want to sign it with your public key.

Ghis

yarikoptic commented 7 years ago

@neurolabusc I haven't realized that github web ui doesn't create those release tags annotated (I guess for the purpose of being able to change/edit release description later on). I guess the only way then would be as @ghisvail pointed out to do it in the command line, and generate github release from the pushed tag (as I usually do)