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

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

Closed
wants to merge 2 commits into from

Conversation

jbpoline
Copy link
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
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
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
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.

description: |
The optional space label (`*[_space-<label>]_electrodes.tsv`) can be used
to indicate the way in which electrode positions are interpreted.
The space label needs to be taken from the list in Appendix VIII.
format: label
split:
name: Split
altname: ''
Copy link
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

@@ -106,6 +117,7 @@ recording:
format: label
proc:
name: Processed (on device)
altname: ''
Copy link
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

@@ -1,11 +1,13 @@
---
sub:
name: Subject
altname: subject
Copy link
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
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
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
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
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
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
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
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
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