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

REL: v1.5.0 #736

Merged
merged 2 commits into from
Feb 23, 2021
Merged

REL: v1.5.0 #736

merged 2 commits into from
Feb 23, 2021

Conversation

franklin-feingold
Copy link
Collaborator

PR marks the start of our 1-week freeze period to prepare to release a new version of the standard (please refer to our release protocol)

This release will officially extend BIDS to support ASL and qMRI imaging

@sappelhoff
Copy link
Member

@patsycle
Copy link
Collaborator

Can we still do minor changes in BEP005 during this freeze period? If yes, what is our deadline?

@sappelhoff
Copy link
Member

Can we still do minor changes in BEP005 during this freeze period? If yes, what is our deadline?

if by "minor changes" you mean bug fixes, then these are still allowed until the release. New features will have to wait for the next release and must then be backwards compatible.

@marcelzwiers
Copy link

I don't know if it is not too late for this, but I would like to propose to rename the entity key in the entities.yaml file (as mentioned also here: #720 (comment)). Because now we have e.g.

inversion:
  name: Inversion Time
  entity: inv
  description: |
    [..]

Which is somewhat strange / confusing because inv is not an entity but just the entity key. Could we change this to:

inversion:
  name: Inversion Time
  key: inv
  description: |
    [..]

In that way getting data from the entities.yaml file is also much cleaner. Instead of the strange looking

entities["inversion"]["entity"] == "inv"

we can then do the much nicer

entities["inversion"]["key"] == "inv"

@patsycle
Copy link
Collaborator

Hi @sappelhoff @franklin-feingold When is the deadline for the release? We are still discussing the requirement level of 1 ASL field, and would have a short TCON within our group on Friday. But would we still be in time to have this included in the current release?

@sappelhoff
Copy link
Member

When is the deadline for the release? We are still discussing the requirement level of 1 ASL field, and would have a short TCON within our group on Friday. But would we still be in time to have this included in the current release?

The release is planned for today, what is the exact issue?

@patsycle
Copy link
Collaborator

for

TotalAcquiredVolumes is now optional but recommended in some cases. However, it appears to be more important for analysis than expected (based @HenkMutsaerts tests), so it should be come requried and a slight change in name an definition. This also implies some small changes in validator.

@sappelhoff
Copy link
Member

I think you could turn it to required now, prior to the release (today), and then do fine-tuning for the next release. But I have also asked the other maintainers about their opinion.

Until we know an answer, it'd be helpful if you could prepare a complete PR so that we see the scope and can then (potentially!) hit the merge button before a release.

But requirement levels are usually something that should be well thought through, as these are hard to change in the future with regards to backwards compatibility.

@sappelhoff
Copy link
Member

I don't know if it is not too late for this, but I would like to propose to rename the entity key in the entities.yaml file (as mentioned also here: #720 (comment)). Because now we have e.g.

@marcelzwiers thanks for the input. Just to put your mind at ease --> the schema is under heavy development and will continue to stay in flux. The forthcoming release will not prevent us from potentially making the adjustments you suggest, because the schema is currently just representing the specification, that is it's (currently) not the specification itself. So we don't have backward compatibility issues yet.

@patsycle
Copy link
Collaborator

I think you could turn it to required now, prior to the release (today), and then do fine-tuning for the next release. But I have also asked the other maintainers about their opinion.

Until we know an answer, it'd be helpful if you could prepare a complete PR so that we see the scope and can then (potentially!) hit the merge button before a release.

But requirement levels are usually something that should be well thought through, as these are hard to change in the future with regards to backwards compatibility.

I'll do these changes today! keep you posted

@HenkMutsaerts
Copy link
Collaborator

HenkMutsaerts commented Feb 23, 2021 via email

@patsycle
Copy link
Collaborator

I think you could turn it to required now, prior to the release (today), and then do fine-tuning for the next release. But I have also asked the other maintainers about their opinion.

Until we know an answer, it'd be helpful if you could prepare a complete PR so that we see the scope and can then (potentially!) hit the merge button before a release.

But requirement levels are usually something that should be well thought through, as these are hard to change in the future with regards to backwards compatibility.

Did the changes in BEP005 but stuck on the PR part now..

@effigies
Copy link
Collaborator

I think you could turn it to required now, prior to the release (today), and then do fine-tuning for the next release. But I have also asked the other maintainers about their opinion.
Until we know an answer, it'd be helpful if you could prepare a complete PR so that we see the scope and can then (potentially!) hit the merge button before a release.
But requirement levels are usually something that should be well thought through, as these are hard to change in the future with regards to backwards compatibility.

Did the changes in BEP005 but stuck on the PR part now..

Created: #742

Assuming that's what you were stuck on...

@franklin-feingold franklin-feingold merged commit 0c30169 into master Feb 23, 2021
@sappelhoff sappelhoff deleted the rel/1.5.0 branch March 30, 2021 09:24
@sappelhoff sappelhoff added the exclude-from-changelog This item will not feature in the automatically generated changelog label Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-from-changelog This item will not feature in the automatically generated changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants