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

[FIX] Deprecate ScanDate (PET) in favor of AcquisitionTime in scans.tsv files #798

Merged
merged 4 commits into from
Jul 13, 2021

Conversation

mnoergaard
Copy link
Collaborator

@mnoergaard mnoergaard commented May 20, 2021

This PR updates the description of the ScanDatefield in the PET specification to be in accordance with the correct units for dates. The updates have been discussed here: bids-standard/bids-validator#1283 (comment)

and the corresponding PR for the validator can be found here: bids-standard/bids-validator#1290

@mnoergaard mnoergaard changed the title [FIX] Update scandate description [FIX] Update ScanDate description for PET May 20, 2021
sappelhoff
sappelhoff previously approved these changes May 20, 2021
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would be interested in what others think.

@mnoergaard mnoergaard requested a review from rwblair May 20, 2021 10:10
@Remi-Gau
Copy link
Collaborator

In summary, it mentions the "official" way to write units for dates with the extra that time is optional in this case.

I did not remember we had kept that piece of metadata for PET and thought we had fallen back on using scans.tsv to keep track of that. Doesn't that open the door for this piece of metadata to be stored in 2 different places and also have potentially conflicted info if that piece of information is stored in both places? (Sorry to have missed that one before the final merge and sorry to not remember how this discussion was resolved... Feel free to ignore).

@mnoergaard
Copy link
Collaborator Author

Yeah - that's the alternative. I guess it would be most clean to have everything in the scans.tsv/json file.

@sappelhoff sappelhoff dismissed their stale review May 21, 2021 07:37

better alternative to use scans.tsv

@effigies
Copy link
Collaborator

+1 for deprecating ScanDate and recommending using acq_time in scans.tsv.

@VisLab
Copy link
Member

VisLab commented May 25, 2021

Putting in scans.tsv is a better alternative -- it allows tool builders to consistently look for the time irrespective of modality

@mnoergaard
Copy link
Collaborator Author

Thank you all - I believe we seem to have a consensus. I will go ahead and make the necessary changes to the specification and the validator, and then make a PR.

@effigies
Copy link
Collaborator

effigies commented Jun 2, 2021

We need to keep the field around for backwards compatibility and mark it deprecated. I made a proposal in edf0237.

@sappelhoff sappelhoff mentioned this pull request Jul 5, 2021
27 tasks
@sappelhoff
Copy link
Member

I made a proposal in edf0237.

@effigies it's a bit unclear to me where that commit is. Is it on your fork? Maybe you could make a PR to Martin's branch (or push directly since he's agreed with your suggestion)

image

@sappelhoff sappelhoff changed the title [FIX] Update ScanDate description for PET [FIX] Deprecate ScanDate (PET) in favor of AcquisitionTime in scans.tsv files Jul 13, 2021
@sappelhoff sappelhoff merged commit 182e828 into bids-standard:master Jul 13, 2021
@sappelhoff
Copy link
Member

Thanks @mnoergaard for starting this and everybody else for reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants