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

[SCHEMA] Update entity YAML keys #714

Merged
merged 2 commits into from
Jan 29, 2021
Merged

[SCHEMA] Update entity YAML keys #714

merged 2 commits into from
Jan 29, 2021

Conversation

effigies
Copy link
Collaborator

In keeping with #616, updating the keys to "common concepts" for inv, res, den and desc. I did not do mt.

Update inversion, resolution, density and description.

Skipping mt for now...
@sappelhoff sappelhoff requested a review from tsalo January 23, 2021 08:32
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM!

@effigies
Copy link
Collaborator Author

Sorry, threw this together immediately before a meeting, so didn't have time to write much, and was kind of hoping the discussion would happen in the absence of prompts. Did we want inversion to be inversion_time or some such? What about mt?

Since the schema is intended to be used by other tools, getting a pretty definite policy early would be good so we don't pull the rug out from under anybody in the future.

@tsalo
Copy link
Member

tsalo commented Jan 25, 2021

Sorry, threw this together immediately before a meeting, so didn't have time to write much, and was kind of hoping the discussion would happen in the absence of prompts. Did we want inversion to be inversion_time or some such? What about mt?

I prefer inversion, since we have the echo pattern already. As for mt, I don't have a good answer, but @agahkarakuzu might be able to weigh in? My first though is magtrans, which isn't great.

@agahkarakuzu
Copy link
Contributor

@effigies mapping between entity and parameters are inv --> InversionTime and mt --> MTState.

Instead of using inv, inversion sounds feasible if you are comfortable with longer entity names.

As for mt, I cannot think of a single word that can represent it. Maybe something like magntx if tx-->transfer is not too encryptic, or magntran, not sure.

@sappelhoff
Copy link
Member

I prefer inversion, since we have the echo pattern already.

agreed

as for mt, I think mt is fine. magntran isn't pretty but sounds okay as well.

@agahkarakuzu
Copy link
Contributor

@sappelhoff @tsalo @effigies how would you like to proceed with these entities? We'll need to convert some datasets according to the latest changes, that'd be great if we could stabilize the entity names soon.

@tsalo
Copy link
Member

tsalo commented Jan 29, 2021

@agahkarakuzu sorry for the confusion. The names we're discussing don't impact the actual entities as they're used in datasets, so you're all set. They mostly impact how we recommend that folks refer to the entities in code (e.g., with argument names and internal variable names).

EDIT: As an example, in pybids you'd find files with a given acq value by searching layout.get(acquisition="X") rather than layout.get(acq="X"), and to get a list of acq values across the dataset you'd use layout.get_acquisitions() rather than layout.get_acqs().

@agahkarakuzu
Copy link
Contributor

Thank you for the clarification @tsalo ! I guess I'll consider these while extending the validator then?

@tsalo
Copy link
Member

tsalo commented Jan 29, 2021

I believe that the "full" names are relevant in the validator, although the actual regular expressions still use the entity keys (the "short" names). Sorry to say, but I do so little with the validator that I can't remember for sure.

@effigies
Copy link
Collaborator Author

Thanks for clarifying, @tsalo. Yes, this is just a consideration that what names we use here are very likely to become the names that tools use internally for the variables that are populated by parsing entities, as they migrate to depending on the schema. The multi-word concepts just need a sensible rule that we can apply consistently.

If we follow the precedent of ceagent (entity ce, so must start with ce, use agent as a noun), then mt would be mtransfer. Would that be readable for people?

@tsalo
Copy link
Member

tsalo commented Jan 29, 2021

What about mtstate?

@effigies
Copy link
Collaborator Author

That would be fine with me. Is there any chance of collision with the MTState metadata variable?

@agahkarakuzu
Copy link
Contributor

I think mtransfer is quite clear.

@tsalo
Copy link
Member

tsalo commented Jan 29, 2021

You're totally right. I forgot that it's a rule in the specification that entities and metadata fields not duplicate one another, regardless of capitalization. I don't think we've applied that to the variable names of the entities, but I don't see why we should complicate things. I vote for mtransfer then.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Re-approving just to make my support clear 😄

@effigies
Copy link
Collaborator Author

@tsalo @sappelhoff I'll leave this to you to merge when you think appropriate. It doesn't fall under our usual 5 day rule, but I also don't see a need to do it quickly.

@tsalo tsalo merged commit e21c2c1 into master Jan 29, 2021
@effigies effigies deleted the schema/entity_keys branch January 29, 2021 20:03
@sappelhoff sappelhoff changed the title SCHEMA: Update entity YAML keys [SCHEMA] Update entity YAML keys Feb 13, 2021
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.

5 participants