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

Overhaul Ontology Annotations #120

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Overhaul Ontology Annotations #120

merged 4 commits into from
Sep 11, 2024

Conversation

HLWeil
Copy link
Member

@HLWeil HLWeil commented Sep 6, 2024

I overhauled the specificiation of Ontology Annotations in Annotation tables. Now the specification tries to be more specific about when it talks about the header cells or the value cells. Previously, this was very confusing (see #115) or even ambiguous.

Please @UrsulaE, @StellaEggels, @Hannah-Doerpholz, @Freymaurer take a look regarding correctness of terminology and also clarity of formulation.

Check it out here: https://github.com/nfdi4plants/ARC-specification/blob/fixes/ISA-XLSX.md#ontology-annotations (So markdown is formatted)

As usual, @Brilator @muehlhaus @chgarth

closes #114 in short fix
closes #115

ISA-XLSX.md Outdated

The value in the main column MUST contain the name of the ontology term.

The value in the `Term Source REF` column MUST either contain a short identifier for the `IDSPACE`, which identifies the ontology the term can be found in, or be left empty.
Copy link
Member

Choose a reason for hiding this comment

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

maybe easier: "the ontology containing the term"

@StellaEggels
Copy link

Two small things I would change:

  1. It says to use : , but in the examples sometimes and are separated by ":" and sometimes by "_". I would always use ":".
  2. For Parameters, the example is "time" with Kelvin as a unit, so either change the parameter to temperature or change the unit.

@StellaEggels
Copy link

regarding 1.: I meant between ID SPACE and LOCAL ID (the words I copied didn't show...)

@Hannah-Doerpholz
Copy link

It might be good to also add that in case of columns with units, that the "main" term column is not immediately followed by the Term Source REF and Term Accession Number columns. Maybe just with a pointer that this is described in the next section. Maybe like this (line 695):
Where a value is an Ontology Annotation in an annotation table, Term Accession Number and Term Source REF columns MUST follow the main column, unless the main column contains a unit. In that case refer to the next section.

@HLWeil
Copy link
Member Author

HLWeil commented Sep 9, 2024

Two small things I would change:

  1. It says to use : , but in the examples sometimes and are separated by ":" and sometimes by "_". I would always use ":".
  2. For Parameters, the example is "time" with Kelvin as a unit, so either change the parameter to temperature or change the unit.

Q: "When will the world end?"
A: "I don't know exactly, but it will probably end in 10000 Kelvin"

@HLWeil
Copy link
Member Author

HLWeil commented Sep 9, 2024

Thanks a lot for your inputs, @Brilator @StellaEggels @Hannah-Doerpholz!

Could you take another look whether your remarks were incorporated as intended?

ISA-XLSX.md Outdated
The header of the main column MUST contain the structural column type followed by the `name` of the ontology term in `[]` brackets.
There SHOULD be a `space` between the column type and the `[` bracket.

The headers of the two annotation columns SHOULD contain further ontological information about the ontology term of the main header. In this case, following the static header string, separated by a single space, there MUST be a short ontology term identifier formatted as CURIEs (prefixed identifiers) of the form `<IDSPACE>:<LOCALID>` (specified [here](http://obofoundry.org/id-policy)) inside `()` brackets.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the header term is freetext? You are using MUST here? what is the alternative?

Copy link
Member Author

@HLWeil HLWeil Sep 11, 2024

Choose a reason for hiding this comment

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

Then it's not an ontology term and none of this applies 😅

Edit Note: After a short meeting, I see that the original comment was referring to the case where the value is an ontology term but the header is only a name. Then the brackets should be left empty (fixed now).

The value in the `Term Source REF` column MUST either contain a short identifier for the `IDSPACE`, which identifies the ontology containing the term, or be left empty.

The value in the `Term Accession Number` column MUST either contain a value formatted in one of the following formats, or be left empty:
- `LOCALID` of the ontology, which is only applicable if the matching `IDSPACE` is given in the `Term Source REF` column
Copy link
Contributor

Choose a reason for hiding this comment

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

LOCALID => do we support this in ARCtrl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, will open an Issue

ISA-XLSX.md Outdated

This implements `Ontology Annotation` from the ISA Abstract Model.
> Note: In this example, the value in the `Term Accession Number` column is formatted as a `URL`, but shortened for the purpose of markdown-formatting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note

Github has a very nice markdown feature for notes 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like this deserves its own Issue as a discussion. OFc they look nice in GitHub but weird when not formatted in other Markdown editors.

Also there's a full spectrum of alerts than can be used. But we should use them unified across the specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay will implement it here in the isa-xlsx specs. But we still need some unification of the work Notes in the main specification file. As it's used there for actual notes but also for the specification itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay made the change. Not sure whether the side-notes are now too attention-grabbing though 😅

@HLWeil HLWeil merged commit 3904fb1 into dev Sep 11, 2024
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