Skip to content

restructure#6

Open
tabbassidaloii wants to merge 1 commit into
NL-BioImaging:mainfrom
tabbassidaloii:dev
Open

restructure#6
tabbassidaloii wants to merge 1 commit into
NL-BioImaging:mainfrom
tabbassidaloii:dev

Conversation

@tabbassidaloii

Copy link
Copy Markdown
Collaborator

defining the annotation groups and restructuring the (sub)folders, adding some linkml examples

defining the annotation groups and restructuring the (sub)folders, adding some linkml examples

@gouttegd gouttegd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed the newly added schemas for LinkML correctness, please find my comments below.

address:
description: Address of the organisation
required: false
multiline: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no such thing as a multiline slot in LinkML.

organisation_identifier:
description: Unique identifier for the organisation
required: false
identifier: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You cannot have simultaneously identifier: true and required: false. An identifier slot is always automatically required.

annotation_group: shared
module_name: person
metadata_profile_source: JERM/SEEK + ISA
imports:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is already an imports statement near the top of the file (line 6). A schema can have only one such statement.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In fact, a large part of the header section in that file is duplicated.

metadata_profile_source: JERM/SEEK + ISA
imports:
- linkml:types
- ../ontology/ontology_annotation.linkml.yaml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The filename should not include the .yaml extension – it is automatically appended by the LinkML schema parser.

person_identifier:
description: Unique identifier for the person
required: true
identifier: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing strictly wrong here, but note that required: true is redundant: since the slot is marked as being an identifier, it is already automatically considered as required.

Person:
description: Reusable person metadata entity
slots:
- person_identifier

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As a general rule, I recommend using local, class-specific attributes rather than global, schema-wide slots, if the slots are only ever used in a single class.

It’s much more straightforward, and it also dispenses from having to use a slot name that needlessly repeats the name of the class it belongs to (as in person_identifier).

link_identifier:
description: Identifier for the external link
required: false
identifier: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

link_identifier is always required by virtue of identifier: true, so required: false will be ignored.

- url
- link_url

link_type:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is the expected type of value for that slot? Shouldn’t there be a enum here?

link_description:
description: Description of the external link
required: false
multiline: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

multiline is not a valid attribute for a LinkML slot.

license: https://creativecommons.org/licenses/by/4.0/
imports:
- linkml:types
- ../agents/person.linkml.yaml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The .yaml extension should be removed.

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.

2 participants