Multiple changes: - Githubissues

mwengren commented 5 years ago

removed QARTOD 'flag_method' attribute; added QARTOD std names
clarified Platform guidelines (sensor-per-dataset)
added 'ioos_ingest' attribute
fixed/added examples
updated index.md readability and content
improved side navigation

jessicaaustin commented 5 years ago

@mwengren

Overall these changes look good. Some comments:

(nitpicky) For clarification, change:
- Single-sensor datasets: although multi-platform datasets are disallowed in the IOOS Metadata Profile, it does not disallow breaking an individual platform out into a single dataset per sensor.
to:
- Single-sensor datasets: although multi-platform datasets are disallowed in the IOOS Metadata Profile, it is acceptable to provide data for an individual platform across multiple datasets. Some IOOS RAs use this pattern already, and provide one dataset for sensor package (e.g., CTD, ADCP, met, etc).

In the table, platform_id is not required. So should we explicitly say, if no platform_id is defined in the dataset, we'll assume the dataset is specifying data for only one platform?

Do we want to remove the platform variable from the table?? If we have platform_id and platform_name, I think that's all we need out of what was originally defined in the platform variable. If we're not going to use it then it only confuses things leaving it in there.

I think you should move the full list of QARTOD standard names out of the GTS Ingest section and in to the QARTOD section above. The only flag required for GTS ingest is the rollup flag, so imho it's confusing to have the full list here.

We still need to update all the example and gold standard links, that's on us (and can be in a separate PR).

mwengren commented 4 years ago

@jessicaaustin I added most of those changes. Let me know what you think. Hopefully it is close to done if all goes well with NDBC etc.

The one change I didn't make was the one to platform (variable) and the 'Platform Variable'. It's confusing, but I believe these are necessary still for CF compliance, even if the 'Platform Variable' in our profile is just a shell that only holds the cf_role attribute. @kwilcox and @shane-axiom pointed this out at one point when we were considering dropping them, maybe they can confirm. It would be nice to eliminate them, but I think we'd run afoul of CF rules.

jessicaaustin commented 4 years ago

@mwengren A few things

In the changelog, we have "Make creator_institution, creator_url, license, publisher_url, and summary required". But license and summary are not required in the table below. Do you remember where we landed on that?
I see standard_name_uri as a new thing, but I don't have any memory of discussing it. (and I couldn't find anything in trello or email). I see it was added by @shane-axiom in april (https://github.com/ioos/ioos-metadata/commit/b84f703e23742f968a0340f67106f7b3f8ce676a) but I don't remember where this is coming from.

I made a bunch of other little tweaks, but I can't push to your repo so it shows up here. Can you give me access to your repo? Or I can just email you a patch file. Summary of those changes:

remove a few dangling references to info_url
cleanup examples
changes to QARTOD section for clarity
Simplify rollup flag section of GTS ingest guidelines

mwengren commented 4 years ago

@jessicaaustin standard_name_uri was suggested by Derrick, and I think we discussed at some point, but I don't remember the details. I found an email thread between me and him that I can forward you - I think it was inspired partly by the urn attribute that Axiom had been using independently, so sort of an IOOS-recommended alternative to that.

On the license and summary I'm not sure how that came about, I think Bob mentioned at one point they're required in ERDDAP so therefore we might as well make them required as well. Don't recall if we discussed or not. I may have forgotten to update the table itself.

Can you make a PR against my quartod_fixes branch, and then I'll merge that if it looks ok and then merge both upstream via this PR? I haven't looked at your changes yet but this seems like the easiest approach.

jessicaaustin commented 4 years ago

@mwengren https://github.com/mwengren/ioos-metadata/pull/2

Yes, looks like license and summary are required in ERDDAP. I updated the table (in the PR above) to mark them as required.

Thanks for the explanation of standard_name_uri. That makes sense, I think I just hadn't seen the discussion before. I was going through and checking our ERDDAP server against each attribute in the profile here, and that's when I noticed it. So I'll change our urn to standard_name_uri, and then I think we're fully compliant here.

jessicaaustin commented 4 years ago

From the call today: where we reference standard name table v71, we should add a note that links to https://github.com/cf-convention/cf-conventions/issues/216 . We can remove the note once v71 is released.

mwengren commented 4 years ago

Ok, I think we're looking good here. Made a few minor editorial changes and a few proofreading fixes in the most recent commit.

I think this is ready to merge. Will do this this afternoon. I did think of a few questions I'll email the ERDDAP group offline about, but I don't think they should hold up finally merging this and getting the ball rolling on next steps like updating Compliance Checker and outreach to the RAs, hopefully.

ioos / ioos-metadata

Multiple changes: #9