Conversation
|
@lubitchv Could you also add the new tsv file and the .properties file. |
|
@ofahimIQSS The new geospatial block is an extended version of an old geospatial.tsv. I can upload the new block as geospatial.tsv which will replace the old one or I can add it as a geospatial_new.tsv. In this case it may fail some tests since it is the extension of an old one and hence may conflict with it. What is better way to do? I think the end goal is update the existing block but I am not sure at this stage what should be done. The new block does not need SQL migration script, since it is extension of old. I wrote an instruction in docs (in this PR) how to update existing installation with this extension of geospatial block. |
|
Thanks, @lubitchv ! We can go ahead and use geospatial.tsv as-is—no need to rename it. We'll test it on our end and roll it out accordingly. |
|
I updated geospatial.tsv to a new expanded one. I also updated geospatial property file with new fields and also solr schema.xml I see that integration test is failing. Can I have a name of the test that fails? |
pdurbin
left a comment
There was a problem hiding this comment.
Some feedback based on the latest commits. This PR is looking good!
| .. code-block:: javascript | ||
|
|
||
| curl http://localhost:8080/api/admin/datasetfield/load -H "Content-type: text/tab-separated-values" -X POST --upload-file geospatial_new.tsv | ||
| curl "http://localhost:8080/api/admin/index/solr/schema" > new.xml | ||
| ./dataverse/conf/solr/update-fields.sh /usr/local/solr/solr-9.8.0/server/solr/collection1/conf/schema.xml new.xml | ||
| curl "http://localhost:8983/solr/admin/cores?action=RELOAD&core=collection1" |
There was a problem hiding this comment.
@lubitchv these upgrade instructions are great, thanks. Let's move them to a release note snippet.
There was a problem hiding this comment.
I created the release snippet for upgrade.
| controlledvocabulary.spatialResolutionType.distance=distance | ||
| controlledvocabulary.spatialResolutionType.vertical=vertical | ||
| controlledvocabulary.spatialResolutionType.angulardistance=angularDistance | ||
| controlledvocabulary.spatialResolutionType.levelofdetail=levelOfDetail |
There was a problem hiding this comment.
Here at the bottom let's track failing tests. This is what I'm seeing right now:
edu.harvard.iq.dataverse.api.DatasetFieldsIT.testListAllFacetableDatasetFields
edu.harvard.iq.dataverse.api.DatasetTypesIT.testUpdateDatasetTypeLinksWithMetadataBlocks
edu.harvard.iq.dataverse.api.DataversesIT.testListMetadataBlocks
edu.harvard.iq.dataverse.api.MetadataBlocksIT.testListMetadataBlocks
From a quick look they all have to do with the number of fields expected, like this:
JSON path data.size() doesn't match.
Expected: <64>
Actual: <71>
There was a problem hiding this comment.
I fixed the integration tests.
| - Computational Workflow Metadata (`see .tsv <https://github.com/IQSS/dataverse/blob/master/scripts/api/data/metadatablocks/computational_workflow.tsv>`__): adapted from `Bioschemas Computational Workflow Profile, version 1.0 <https://bioschemas.org/profiles/ComputationalWorkflow/1.0-RELEASE>`__ and `Codemeta <https://codemeta.github.io/terms/>`__. | ||
| - Archival Metadata (`see .tsv <https://github.com/IQSS/dataverse/blob/master/scripts/api/data/metadatablocks/archival.tsv>`__): Enables repositories to register metadata relating to the potential archiving of the dataset at a depositor archive, whether that be your own institutional archive or an external archive, i.e. a historical archive. | ||
|
|
||
| - `New updated Geospatial Metadata block <https://docs.google.com/spreadsheets/d/1GMVIaQB6EqauNDqrPq09BBHd_HzUSD57oliTAoh3Ikw>`__: adapted for ISO 19115-3 format. It substitutes and expands existing geospatial.tsv metadata block. To use it, replace existing geospatial.tsv with the new one. To upload the block to the existing installation: |
There was a problem hiding this comment.
Let's check with @jggautier and others but I believe we we should overwrite "Geospatial Metadata" above rather than having a separate entry. In a release note snippet we can explain that the geospatial block has been updated.
On a related note, rather than "New expanded geospatial block" should the title of this PR be "Updated geospatial block" or "updated" or "improved" or "extended" or whatever. It's not really new, is it? It's bigger and better. 😄
| eastLongitude Easternmost (Right) Longitude Easternmost coordinate delimiting the geographic extent of the Dataset. A valid range of values, expressed in decimal degrees, is -180.0 <= East Bounding Longitude Value <= 180.0. text 8 FALSE FALSE FALSE FALSE FALSE FALSE geographicBoundingBox geospatial | ||
| northLatitude Northernmost (Top) Latitude Northernmost coordinate delimiting the geographic extent of the Dataset. A valid range of values, expressed in decimal degrees, is -90.0 <= North Bounding Latitude Value <= 90.0. text 9 FALSE FALSE FALSE FALSE FALSE FALSE geographicBoundingBox geospatial | ||
| southLatitude Southernmost (Bottom) Latitude Southernmost coordinate delimiting the geographic extent of the Dataset. A valid range of values, expressed in decimal degrees, is -90.0 <= South Bounding Latitude Value <= 90.0. text 10 FALSE FALSE FALSE FALSE FALSE FALSE geographicBoundingBox geospatial | ||
| geographicCoverage Geographic Coverage Information on the geographic coverage of the data. Includes the total geographic scope of the data. none 0 FALSE FALSE TRUE FALSE TRUE FALSE geospatial |
There was a problem hiding this comment.
Someone at standup noticed that you've changed displayoncreate for a lot of field from false to true:
pdurbin@beamish dataverse % git checkout develop
Switched to branch 'develop'
Your branch is up to date with 'origin/develop'.
pdurbin@beamish dataverse % cat scripts/api/data/metadatablocks/geospatial.tsv | head | cut -f2,13
name
geospatial
name displayoncreate
geographicCoverage FALSE
country FALSE
state FALSE
city FALSE
otherGeographicCoverage FALSE
geographicUnit FALSE
geographicBoundingBox FALSE
pdurbin@beamish dataverse % git checkout 10398-geospatial-block
Switched to branch '10398-geospatial-block'
Your branch is up to date with 'lubitchv/10398-geospatial-block'.
pdurbin@beamish dataverse % cat scripts/api/data/metadatablocks/geospatial.tsv | head | cut -f2,13
name
geospatial
name displayoncreate
geographicCoverage TRUE
country TRUE
state TRUE
city TRUE
otherGeographicCoverage TRUE
geographicUnit TRUE
geographicBoundingBox TRUE
Can you please switch them back to false?
On a related note, there's a new API added in #11001 for 6.6, that allows you to change displayoncreate: https://guides.dataverse.org/en/6.6/api/native-api.html#set-displayoncreate-for-a-dataset-field . So maybe you can use that to set fields to true for your installation.
There was a problem hiding this comment.
I replaced back displayoncreate to all the fields to FALSE
|
Hi all! Just got back from vacation and noticed I was asked if this metadata block should be described in the "Experimental Metadata" section or if its description should replace what's in the "Supported Metadata" section about geospatial metadata. @lubitchv could you write why you added a description in the "Experimental Metadata" section? From the discussion in this PR so far I couldn't get a sense of why. And where ever the metadata block is described on this Appendix page, should it include information about how to upload the metadata block? I ask because this would be the first time we include that sort of information here, when I think it's usually in the release notes and in the Metadata Customization section of the Admin Guide. |
|
Just an update from my comments and questions yesterday after I got to chat briefly with @lubitchv this morning at the Dataverse Community Meeting at UNC. We agreed to try to find time to discuss more with more folks, including @amberleahey. I should also say that I just noticed that "It is a new experimental geospatial block that extends the old one" is what's in the "Special notes for your reviewer" section of this PR's first comment, so it sort of makes sense to me why you'd want to describe it in the Appendix's "Experimental Metadata" section. And I think it would help to discuss with @pdurbin why he's encouraging you to instead replace what's in the Appendix's "Supported Metadata" section. Hopefully we'll find time to chat this week during the conference. |
|
Absolutely! Active is better! And bringing in @emily-katz and @Saixel from CAFE is an excellent idea. 💡 I'm happy to join a meeting if that helps. This PR changes the actual, real, production geospatial metadata block so once it's merged it is very much non-experimental. Perhaps we could use the word "preview" instead? 🤔 Whatever gets across the idea that we'd like people to try the updated metadata block before we merge. 😄 |
|
Moved to hold |
|
Here's the already-merged PR that updated the appendix to encourage people to try the version of the geospatial block in this PR (#11507): At some point @amberleahey and @lubitchv will have enough feedback that they'll make modifications or ask to move this PR forward as-is. |
Update from my synced develop
|
Can you let me know what integration test is failing? |
|
It's a build issue - I merged develop into your branch to see if that helps. |
|
Made a comment on #1592 |
| country Zimbabwe 247 | ||
| country Åland Islands 248 | ||
| #metadataBlock name dataverseAlias displayName | ||
| geospatial Geospatial Metadata |
There was a problem hiding this comment.
Have you considered a blockUri and/or termUri entries to make the association with 19115 clearer? As an XML standard they may not have a good mapping to URIs/RDF, but it might still make sense to add URI mapping even if they are of the form https://dataverse.org/vocab/iso19115/* (especially if not all fields in the block map to 19115)
What this PR does / why we need it: Expanded geospatial block is proposed
Which issue(s) this PR closes:
Special notes for your reviewer:
It is a new experimental geospatial block that extends the old one.