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] harmonize and thus shorten templates etc #93

Merged
merged 14 commits into from
Nov 28, 2018

Conversation

yarikoptic
Copy link
Collaborator

@yarikoptic yarikoptic commented Nov 20, 2018

Otherwise all of those are used inconsistently, make templates longer for no apparent benefit, etc,

IMHO there is not need to capitalize it in the text.  Moreover it was
inconsistent - some places had it capitalized and some not
…- implied

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi 'ses-<session_label>' 'ses-<label>'",
 "exit": 0,
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
…label> -- implied (and inconsistent)

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi 'sub-<participant_label>' 'sub-<label>'",
 "exit": 0,
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
…lied

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi 'task-<task_label>' 'task-<label>'",
 "exit": 0,
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi 'run-<run_index>' 'run-<index>'",
 "exit": 0,
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "git-sedi 'dir-<dir_label>' 'dir-<label>'",
 "exit": 0,
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
* origin/master:
  Change BEP numbers to include MRS
  ENH: allow _dir for other EPI (func, dwi) sequences

Conflicts:
    src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
       as promised -- was conflicting with my changes in other PR.  Tuned it
       all up so there is no dir_label now
@yarikoptic
Copy link
Collaborator Author

Dear @bids-standard/everyone (seems a designated team with "merge" privileges would be useful to have) -- please provide your feedback and/or merge.

Cheers!

Copy link
Contributor

@chrisgorgo chrisgorgo left a comment

Choose a reason for hiding this comment

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

Personally I like the verbosity of the original names and don't see the benefit for shortening the templates, but it's just a subjective opinion.

@@ -142,7 +142,7 @@ run labels). When there is only one scan of a given type the run key MAY be
omitted. Please note that diffusion imaging data is stored elsewhere (see
below).

The OPTIONAL `acq-<label>` key/value pair corresponds to a custom label the user
The optional `acq-<label>` key/value pair corresponds to a custom label the user
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why lowercase OPTIONAL? It's supposed to be an indication that this is a RFC2119 label. If you are proposing to change that it should be done in another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ha, also found https://tools.ietf.org/html/rfc8174 ;-) so indeed then should be all UPPERCASE to be keywords.
Again -- I was just aiming for consistency and there were uses both ways, I will just push (later today) a fix to get them all UPPERCASED

@chrisgorgo
Copy link
Contributor

I gave you merge rights - if anyone else wants that please email me.

@yarikoptic
Copy link
Collaborator Author

Personally I like the verbosity of the original names and don't see the benefit for shortening the templates, but it's just a subjective opinion.

The main problem is/was that it was not consistent to any stretch. Some were long, some short, etc, and I believe some both ways (I could be wrong now).

@yarikoptic
Copy link
Collaborator Author

ok, pushed more for consistent use of OPTIONAL and REQUIRED... didn't bother splitting into two PRs to avoid conflicts and messing with already resolved ones (life is too short ;-))

@yarikoptic
Copy link
Collaborator Author

BTW, while doing linting locally I get:

$> node_modules/remark-cli/cli.js src/*.md src/*/*.md --frail
...
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
  277:1-277:24  warning  Do not use headings with similar content (65:1)  no-duplicate-headings  remark-lint
...

$> node_modules/remark-cli/cli.js --version                                        
remark: 9.0.0, remark-cli: 5.0.0

which is due to that Timing Parameters subsection which is imho kosher since sections are different:

### Scanner Hardware
#### Sequence Specifics
#### In-Plane Spatial Encoding
#### Timing Parameters
...
### Task (including resting state) imaging data
#### Required fields
#### Other RECOMMENDED metadata
##### Timing Parameters

how to suppress it? (thought to put that call into precommit hook to avoid keep messing things up)

@yarikoptic
Copy link
Collaborator Author

Dear @bids-standard/everyone , please chime in on this PR which unifies presentation (no "functional" changes here). Due to its "spread" it would quickly accumulate conflicts. Before I spend any further time fixing them I would like to know if I should bother.

Cheers!

@emdupre
Copy link
Collaborator

emdupre commented Nov 27, 2018

Personally I liked the longer, more explicit label names, but I'm happy to roll what whatever makes sense to the broader community. Thanks for thinking about this, @yarikoptic !! ✨

@yarikoptic
Copy link
Collaborator Author

heh, "broader community" seems to be just me here... I took a look back at current specification, and decided to stay firm that such unification would be helpful in the long run, there is now just too much variance (run-<index> and run-<run_index> etc) which cannot be useful. I am going to resolve the conflict and then possibly take advantage of given to me recently merge powers. Ready to bear the blame or pride ;)

* origin/master:
  adding extensions page
  simplify options wrt sparse sequences
  fix table fence
  check table legend
  check table legend
  hopefully last fix to list in volume timing table legend
  additional fix to list in table legend
  wrapping long lines
  BF: fix the stale anchor left from google doc, also add reference at other points
  BF: fixed a url (https -> http)
  change table legend to list
  Fixes long line
  Fixes misaligned table fence 2
  Fixes misaligned table fence
  Fixes misalignement and use of square bracket
  add table for volume timing
  highligh some fields as code, probably missed during the clean up after import from google doc
@yarikoptic yarikoptic merged commit 49721b5 into bids-standard:master Nov 28, 2018
@yarikoptic yarikoptic deleted the rf-harmonize branch December 5, 2018 17:23
@sappelhoff sappelhoff changed the title RF harmonize and thus shorten templates etc [FIX] harmonize and thus shorten templates etc Sep 8, 2020
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.

3 participants