Skip to content

[ENH] Introduce "altnames" for entities as recommendation for variable names in BIDS supporting software#588

Closed
jbpoline wants to merge 2 commits intobids-standard:masterfrom
jbpoline:patch-1
Closed

[ENH] Introduce "altnames" for entities as recommendation for variable names in BIDS supporting software#588
jbpoline wants to merge 2 commits intobids-standard:masterfrom
jbpoline:patch-1

Conversation

@jbpoline
Copy link
Copy Markdown
Contributor

@jbpoline jbpoline commented Sep 1, 2020

edit SA: introduce "alt-names" for entities in schema as RECOMMENDATION on how to name variables in BIDS supporting software. closes #585

some of the "name" dont match pybids - proposing to add a key for this
change pybids_name to altname
@jbpoline
Copy link
Copy Markdown
Contributor Author

jbpoline commented Sep 1, 2020

Also referencing #585

@sappelhoff sappelhoff changed the title altnames in BIDS yml schema [ENH] Introduce "altnames" for entities as recommendation for variable names in BIDS supporting software Sep 1, 2020
Copy link
Copy Markdown
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.

I like the idea, but we should come up with an altname for each entity, for example in the current proposal split still has ''as altname

@tsalo
Copy link
Copy Markdown
Member

tsalo commented Sep 1, 2020

@sappelhoff In the call that led to this PR, one idea was to essentially "inherit" the variable name from the entity name (i.e., convert it to lower-case and then use as-is) when the variable name isn't explicitly set. That said, I agree with you. I'd prefer to be explicit about it, rather than add a new rule.

format: label
split:
name: Split
altname: ''
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
altname: ''
altname: split

format: label
proc:
name: Processed (on device)
altname: ''
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
altname: ''
altname: processed

---
sub:
name: Subject
altname: subject
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I dislike duplication. Why not to make altname to be considered name.lower() if undefined, and define it only if it does not follow that rule?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not to make altname to be considered name.lower() if undefined, and define it only if it does not follow that rule?

no strong feeling, but looking at this it seems easier to simply add an altname everywhere (guarantee there is one). The alternative would be to codify the name.lower() rule somewhere and link to it - but then people have to find that rule, and always make a check "is there an altname?" --- if not, take "name" and apply .lower()

I am aware that the latter of what I said is still pretty easy, hence why I started my sentence with "no strong feeling".

What is your opinion @tsalo ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I generally prefer explicit over implicit. I think there is room for new rules to reduce duplication, but I think we should hold off on determining and implementing those rules until the schema is more stable. Once we have a complete and stable version of the schema, we'll be able to better see what duplication is necessary and what duplication can be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with this - sounds reasonable to me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sounds like we are 3 people in favor of "duplication" here, @yarikoptic do you maybe want to tag some more reviewers that you want to provide their opinion?

Let me tag @effigies

Copy link
Copy Markdown
Collaborator

@effigies effigies Sep 11, 2020

Choose a reason for hiding this comment

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

I'll annoy everybody and say I'm fine with explicitness at the cost of duplication, but that I find "altname" ambiguous in intent.

What we really have are three contexts: 1) Programmatic; 2) File name; 3) Spec text. Given that this document is tooling-centric, maybe the key should be the programmatic name:

ceagent:
    name: Contrast Enhancing Agent
    entity: ce
    description: ...
reconstruction:
    name: Reconstruction
    entity: rec
    description: ...

We could be more explicit and say display-name instead of name and entity-key instead of entity.

WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that's a reasonable change. It definitely makes including a "preferred variable name" less awkward.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 also from my side

@effigies
Copy link
Copy Markdown
Collaborator

I've proposed #616 as an alternative to this, based on recent discussion.

@tsalo tsalo closed this in #616 Oct 1, 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.

Add altname or preferred variable name field to entities.yaml in schema

5 participants