-
Notifications
You must be signed in to change notification settings - Fork 33
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
merge develop to create 1.3 release #207
Conversation
convention for including `spdx:checksum` for objects
Feature 135 jsonld @list keyword
pull MBJones Restructured intro to variables section. into Issue27-forPullRequest branch
…cussion to advanced section
make it clearer that data repository guidance is for describing a repository, not where repositories should necessarily start.
shows examples from other vocabularies with differnt structures. EarthRef does not have URI's associated with terms; SnowTerm (from Chantelle Verhey's list) has URLs for terms, and ARK IDs, but the ARKS don't dereference through the standard N2t.net resolver....
Issue77 temporal coverage
Issue77 temporal coverage
change gstime to gsqtime for NIs
This addresses issue #223, and pulls text from our decision document on namespace consistency. It complements the text in the Dataset.md guide, which now says the same thing but explains it differently.
guides/Dataset.md
Outdated
"time": "http://www.w3.org/2006/time#", | ||
"xsd": "https://www.w3.org/TR/2004/REC-xmlschema-2-20041028/datatypes.html" | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No brace needed here. And delete matching close brace below.
guides/Dataset.md
Outdated
} | ||
} | ||
}]</strong> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete brace
guides/Dataset.md
Outdated
"time": "http://www.w3.org/2006/time#", | ||
"xsd": "https://www.w3.org/TR/200 (from [OWL Time](http://www.w3.org/2006/time))4/REC-xmlschema-2-20041028/datatypes.html" | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete brace and close brace below.
guides/Dataset.md
Outdated
"time": "http://www.w3.org/2006/time#", | ||
"xsd": "https://www.w3.org/TR/2004/REC-xmlschema-2-20041028/datatypes.html" | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete brace and close brace below.
guides/Dataset.md
Outdated
"time": "http://www.w3.org/2006/time#", | ||
"xsd": "https://www.w3.org/TR/2004/REC-xmlschema-2-20041028/datatypes.html" | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete brace and close brace below.
guides/Dataset.md
Outdated
"ts": "http://resource.geosciml.org/vocabulary/timescale/", | ||
"xsd": "https://www.w3.org/TR/2004/REC-xmlschema-2-20041028/datatypes.html" | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete brace and close brace below.
Update namespace guidance to conform with http guideline.
Note that these problems occured due to extra braces that were present when moving the examples from the `@graph` based version in the example file into the markdown document. I checked them in the playground and they seem to work now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review focuses on the new Temporal Coverage section, which looks good. My one request for change is that we eliminate the non-parseable and duplicated array entry that leads off each temporalCoverage array. As a duplicate, harvesters and other consumers do not need this information, and because it is not machine parseable, harvesters won't know how to deal with it and may end up not including temporal coverage in their search. Better to stick with machine parseable representations of temporal coverage, and only use multiple elements in the array when trying to express discontiguous temporalCoverage periods.
|
||
"@type": "Dataset", | ||
"description": "Eruptive activity at Mt. St. Helens, Washington, March 1980 - January 1981", | ||
<strong> "temporalCoverage": ["1980-03-27T19:36:00/1981-01-03T00:00:00Z", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This temporal coverage duplicates the coverage information in the two elements of the array, using different syntax. This is not necessary and is confusing. As we are only illustrating here how time:ProperInterval
works, I suggest the first entry should be deleted.
This problem is repeated in all 7 temporal coverage examples.
<strong> "temporalCoverage": ["1980-03-27T19:36:00/1981-01-03T00:00:00Z", | |
<strong> "temporalCoverage": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbjones This change seems good to me. We discussed having a text field as one element in an array of temporalCoverage entries but we do have a human readable description for the age, so removing it for the reasons you stated makes sense. These changes also need to be made to the decision doc and the temporalCoverage.jsonld example file. I can do that by Tuesday night next week, if someone doesn't want to do it before then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if the array should be removed when there is only one element in the temporalCoverage array. Only example 6 has more than one element in the temporalCoverage array once these duplicate text entries are removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be fine to remove the array, or to leave it as a one-element array. I decided it was simpler to leave it.
}, | ||
"@type": "Dataset", | ||
"description": "Eruption of Bishop Tuff, about 760,000 years ago", | ||
<strong>"temporalCoverage": ["760 ka", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same duplication issue as before, but also this string is not machine parseable following the xsd syntax.
<strong>"temporalCoverage": ["760 ka", | |
<strong>"temporalCoverage": [ |
}, | ||
"@type": "Dataset", | ||
"description": "Very old zircons from the Jack Hills formation Australia 4.404 +- 0.008 Ga (2-sigma)", | ||
<strong>"temporalCoverage": ["4.404 +/-+/- 0.008 Ga", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same duplication issue, and not machine parseable.
<strong>"temporalCoverage": ["4.404 +/-+/- 0.008 Ga", | |
<strong>"temporalCoverage": [ |
}, | ||
"@type": "Dataset", | ||
"description": "Isotopic ages determined at the bottom and top of a stratigraphic section in the Columbia River Basalts", | ||
<strong>"temporalCoverage": ["17.1 +/- 0.15 to 15.7 +/- 0.14 Ma", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same duplication issue, and not machine parseable.
<strong>"temporalCoverage": ["17.1 +/- 0.15 to 15.7 +/- 0.14 Ma", | |
<strong>"temporalCoverage": [ |
}, | ||
"@type": "Dataset", | ||
"description": "Age of a piece of charcoal found in a burnt hut at an archeological site in Kenya carbon dated at BP Calibrated of 2640 +130 -80 (one-sigma) using the INTCAL20 carbon dating curve.", | ||
<strong>"temporalCoverage": ["2640 +130 -80 BP-CAL (INTCAL20)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated coverage, and not machine parseable.
<strong>"temporalCoverage": ["2640 +130 -80 BP-CAL (INTCAL20)", | |
<strong>"temporalCoverage": [ |
}, | ||
"@type": "Dataset", | ||
"description": "Temporal position expressed with a named time ordinal era from [International Chronostratigraphic Chart](https://stratigraphy.org/chart):", | ||
<strong>"temporalCoverage": ["Bartonian", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated coverage, not machine parseable.
<strong>"temporalCoverage": ["Bartonian", | |
<strong>"temporalCoverage": [ |
}, | ||
"@type": "Dataset", | ||
"description": "Temporal position expressed with an interval bounded by named time ordinal eras from [International Chronostratigraphic Chart](https://stratigraphy.org/chart). NumericPositions not included, expect clients can lookup bounds for ISC nominal positions:", | ||
<strong>"temporalCoverage": ["Triassic to Jurassic", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated coverage, not machine parseable.
<strong>"temporalCoverage": ["Triassic to Jurassic", { | |
<strong>"temporalCoverage": [ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the SOSO Cluster call today on 2022-06-06, we reviewed the current proposed changes in the develop branch and voted unanimously to approve the 1.3 release and the endorsement document with the changes discussed during the call, which have now been made to both documents. So @ashepherd, we now have agreement across the group to merge develop
to main
and tag and release it. As the author of this PR, I can not approve it, but once @ashepherd or @datadavev or somebody approves it, we will be able to click merge.
This is the PR for merging all 1.3 release changes into the main branch. Please review and suggest any changes that ar needed before the release can be made. I am opening it now as a location to collate release feedback. The PR incorporates many changes from 1.2, across many previously reviewed Pull Requests, so this is a last opportunity to catch small errors and issues. There are a few release details that still need to be committed to develop before the release is fully ready (such as assigning the new DOI and a few other maintenance issues described in #201 . Once those are committed to the
develop
branch we can proceed to merge this PR.