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

Add Scope, Cardinality, Type, and DictionaryItem classes #6938

Merged
merged 3 commits into from
Dec 9, 2020

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Aug 27, 2020

This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR #6936 but does not use yet use them.

@driusan driusan added Blocking PR should be prioritized because it is blocking the progress of another task Feature PR or issue introducing/requiring at least one new feature labels Aug 27, 2020
Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

just a few comments on design decisions. nothing serious

src/Data/Cardinality.php Show resolved Hide resolved
Comment on lines +40 to +42
* A Many Cardinality signifies that each data point will
* have zero or more values associated. For instance,
* the T1 scans acquired at a session.
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like the example is a bit too specific, why not use something like For instance, site affiliations for a user for us non-imaging people

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't mind changing the example but I think site/project affiliations is a bad example because there's no "user" scoped data, and the example might get confusing since site/project affiliations exist on both candidate and session scopes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about using the candidate/session relationship then ? a candidate can have 0 to many sessions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be confusing too though, because there's the session scope and there's no variable named "Session", and session based data is usually described as 1:1 with a session scope, not 1:many with the candidate.

Comment on lines +36 to +37
Type $t,
Cardinality $c
Copy link
Collaborator

Choose a reason for hiding this comment

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

why single letter variables, i'm not a big fan and in the long run they always tend to be a hindrance (same goes for shortcuts like cand for candidate...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just the argument name in the constructor, the context only lasts long enough to assign it to a property right below.. the property name is the full word.

protected $name;
protected $description;
protected $scope;
protected $typ;
Copy link
Collaborator

Choose a reason for hiding this comment

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

typ ?? not type ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

type is a reserved keyword in php.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at least I thought it was and editor was highlighting it as one.. I'll look into it a little more.

Comment on lines +8 to +10
* The Scope class is final because the list of enumeration types
* can not be dynamically extended without modifying all places
* that must deal with the enumeration options.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems limiting to projects wanting to define their own scopes... why not a database table defining the constants...

would avoid overriding/overloading specially since we dont offer any easy overriding structure for the src directory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Scopes can't be extensible because everywhere in the code that deals with the data needs to understand how to deal with that scope. Let's say a module added a scope named "foobar" and someone accessed that a variable with that scope in the DQT. Questions like "how should this be displayed? How does it relate back to candidates? How does it relate to sessions? Does it need to show a list of sessions to select? How does it get displayed in the results table? How do I join it with candidate scoped data to show in a row?" need to be understood by the code to build the interface and display results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(if you have a suggestion for how to make the comment more clear I'll update it.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

no your right... I'm just thinking if there is a way we can add project defined scopes in the same way. my immediate interest is the Biospecimen scope for CBIGR

Copy link
Collaborator

Choose a reason for hiding this comment

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

it could come later though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are similar problems with things like imaging QC. "Pass/Fail" is really file scoped data but there is no file scope. For now I've defined them as "many" cardinality scoped to the session.

Copy link
Collaborator Author

@driusan driusan left a comment

Choose a reason for hiding this comment

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

for some reason my replies to comments aren't being submitted unless I click submit a review..

src/Data/Cardinality.php Show resolved Hide resolved
Comment on lines +36 to +37
Type $t,
Cardinality $c
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just the argument name in the constructor, the context only lasts long enough to assign it to a property right below.. the property name is the full word.

Comment on lines +40 to +42
* A Many Cardinality signifies that each data point will
* have zero or more values associated. For instance,
* the T1 scans acquired at a session.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be confusing too though, because there's the session scope and there's no variable named "Session", and session based data is usually described as 1:1 with a session scope, not 1:many with the candidate.

This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR aces#6936 but does not use yet use them.
@driusan driusan merged commit 5fa9a04 into aces:main Dec 9, 2020
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
This adds the classes representing the data scope, data type,
data cardinality, DictionaryItem (and dictionary Category) from
PR aces#6936 but does not use yet use them.

It gives us the ability to describe data dictionaries in LORIS.
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking PR should be prioritized because it is blocking the progress of another task Feature PR or issue introducing/requiring at least one new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants