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

Release from staging to master - 2021-03-17 #1371

Merged
merged 123 commits into from
May 5, 2021
Merged

Release from staging to master - 2021-03-17 #1371

merged 123 commits into from
May 5, 2021

Conversation

ESapenaVentura
Copy link
Collaborator

@ESapenaVentura ESapenaVentura commented Mar 17, 2021

Release notes

Versions:

system/links.json - v3.0.0
module/protocol/matrix.json - v1.0.0
type/protocol/analysis/analysis_protocol.json - v9.2.0
type/file/analysis_file.json - v6.3.0
core/file/file_core.json - v6.2.0
type/file/sequence_file.json - v9.3.0
type/file/supplementary_file.json - v2.3.0
type/file/reference_file.json - v3.3.0
type/file/image_file.json - v2.3.0
module/ontology/data_use_ontology.json - v1.0.0
type/project/project.json - v15.0.0

Test metadata:
HumanCellAtlas/schema-test-data#8

Functionality:

  • Fixed javascript validator (Points to correct URL)
  • Added fail_test for links.json
  • Fixed schema linter (Points to correct URL)
  • Fixed pullapprove and updated lists
  • Added test data spreadsheet

Additional notes:

  • type/project/project.json has advanced 1 major version because of a rollback. The major change should have no impact as it involves the deletion of a field that was never used.

ESapenaVentura and others added 30 commits February 22, 2021 16:39
Added pending status while travis test is happening
Tried with success endpoint instead of successful
Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.8.
- [Release notes](https://github.com/isaacs/ini/releases)
- [Commits](npm/ini@v1.3.5...v1.3.8)

Signed-off-by: dependabot[bot] <[email protected]>
…tests/ini-1.3.8

Bump ini from 1.3.5 to 1.3.8 in /tests
Bumps [bl](https://github.com/rvagg/bl) from 4.0.2 to 4.1.0.
- [Release notes](https://github.com/rvagg/bl/releases)
- [Commits](rvagg/bl@v4.0.2...v4.1.0)

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [bl](https://github.com/rvagg/bl) from 4.0.2 to 4.1.0.
- [Release notes](https://github.com/rvagg/bl/releases)
- [Commits](rvagg/bl@v4.0.2...v4.1.0)

Signed-off-by: dependabot[bot] <[email protected]>
…tests/bl-4.1.0

Bump bl from 4.0.2 to 4.1.0 in /tests
Improved description of file_source thanks to Hannes-ucsc.

Co-authored-by: Hannes Schmidt <[email protected]>
@mshadbolt
Copy link
Contributor

mshadbolt commented Mar 30, 2021

Just so the reviewers who have reviewed so far are aware (@hannes-ucsc @ruchim). We have taken your feedback on board and we are now preparing a new feature branch to merge into staging with your suggested fixes.

We are also removing all traces of the data_use_ontology_module and manually rolling back the project versioning and re-releasing the schemas to staging so that it was like the module, and the version of the project schema that referred to it, never existed.

We will also re-generate the test data. Once this is all completed we will re-request review of the release of the modified staging release into master.

Thanks for all your feedback.

Copy link

@kbergin kbergin left a comment

Choose a reason for hiding this comment

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

Had a question about the analysis_protocol json changes. Outside of that question, everything else shouldn't impact analysis as far as I can tell.

Copy link
Contributor

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

A few more comments.

json_schema/type/file/analysis_file.json Show resolved Hide resolved
Comment on lines 56 to 59
"enum":[
"DCP",
"Contributor"
],
Copy link
Contributor

@hannes-ucsc hannes-ucsc Mar 31, 2021

Choose a reason for hiding this comment

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

Suggested change
"enum":[
"DCP",
"Contributor"
],
"enum":[
"DCP/2 Analysis",
"Contributor",
"Array Express",
"HCA Release",
"GEO",
"SCEA",
"SCP",
"DCP/1 Matrix Service",
"LungMAP",
"Zenodo",
"Publication",
],

to be consistent with the set of sources that we currently track (https://github.com/DataBiosphere/azul/blob/6b45e0e30fbe90c7b3944ee691c95328c7883ab2/src/azul/plugins/metadata/hca/transform.py#L271).

It would be undesirable to be less informative than the current "hacky" mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hannes-ucsc it might be nice to also represent if a file source type is "internal" or "external". We could do this by making this an enum of dictionaries instead of strings, with an added property called "type". This would be of use in simplifying to users the distinction between DCP and CGM matrices.

Copy link
Contributor

@mshadbolt mshadbolt Apr 7, 2021

Choose a reason for hiding this comment

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

@NoopDog I did some searching but I can't find anything about having an enum of objects in json schema. Or are you suggesting an additional field in the schema? If it is a 'nice to have' would it be possible to make a ticket on the repo with your proposal and we will prioritise it. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@hannes-ucsc I have incorporated the enums as requested in the PR I am making with all feedback from the review process (#1381). We'll wait until test data has been reviewed and all expected changes are made in this branch before internal review, then squash and merge to minimise commits, then re-publish the schemas on staging.

Copy link
Contributor

@NoopDog NoopDog left a comment

Choose a reason for hiding this comment

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

Hi there, I would like to propose an alternative to making an optional field for matrix_cell_count on analysis_file.json.

Another way to do this is to make a matrix_file.json that includes analysis_file.json, basically extending it and making a new type, and then adding matrix_cell_count as a required field to matrix_file.json.

We already have image_file, reference_file, sequence_file and it seems to me that since matrix files are one of our primary outputs, that we would do well to model them explicitly.

This would provide a location to attach other matrix-related properties in the future and allow the schema to specify how to validate them, and also enable filtering by the file type.

* manually removed evidence of duo module and field

* removed data_use_ontology module

* manually edit project version

* wording changes for matrix module

* Modified descriptions based on feedback

* slight wording change

* add suggested enums

* removed space from array express

* edited examples, guidelines

* Fixed array of enums inclusion.

Co-authored-by: ESapenaVentura <[email protected]>
"description": "Method(s) used to normalize data in the matrix",
"type": "array",
"user_friendly": "Data normalization method(s)",
"items":{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"items":{
"items": {

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a deal-breaker. @clairerye: "Will be fixed before merged". I will post formatting configuration for Amnon.

]
}
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

See formatting fixes above.

Copy link
Contributor

@ruchim ruchim left a comment

Choose a reason for hiding this comment

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

all good

@clairerye clairerye requested a review from aherbst-broad April 29, 2021 19:47
@hannes-ucsc hannes-ucsc requested a review from NoopDog April 30, 2021 00:54
@ESapenaVentura ESapenaVentura merged commit c1e477d into master May 5, 2021
Wkt8 added a commit to ebi-ait/geo_to_hca that referenced this pull request May 11, 2021
Added updated HCA template spreadsheet with the following schema changes
Added EGA and dbGAP accession to Project Tab
Added Analysis File and Analysis Protocol Tabs
Added File_source to Analysis File tab
Added Matrix_Cell_Count to Analysis File Tab
Added Derivation_process and data_normalization_method to Analysis Protocol tab 

See schema change PR here: HumanCellAtlas/metadata-schema#1371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Any PR that incorporates changes to the schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.