GenomicsStandardsConsortium / mixs

Minimum Information about any (X) Sequence” (MIxS) specification
https://w3id.org/mixs
Creative Commons Zero v1.0 Universal
33 stars 20 forks source link

`recommended` attribute not removed from terms/slots when the cardinality value in the docs is '1..1' (indicating `required`) #672

Open mslarae13 opened 8 months ago

mslarae13 commented 8 months ago

Describe the bug A clear and concise description of what the bug is.

In the documentation, depth is optional. EXCEPT when in soil.. it's required.. but when you look at a MimsSoil combination, the cardinality holds for soil (1...1) but the recommended doesn't go away

Same for elevation

To Reproduce Steps to reproduce the behavior:

  1. Go to https://genomicsstandardsconsortium.github.io/mixs/Soil/

Expected behavior A clear and concise description of what you expected to happen.

recommended will disappear when cardinality is 1..1

Screenshots If applicable, add screenshots to help explain your problem.

Screenshot 2023-10-18 at 9 43 13 PM
sujaypatil96 commented 8 months ago

So what's happening here is that depth has been modified (using slot_usage) under both the Mims checklist as well as the Soil extension.

So when the combination is made from both Mims checklist and Soil extension, both the required and recommended properties are getting propagated, and like we would expect the documentation is reflecting that.

It is possible that a term can be both required (by the schema) and recommended (based on expert guidance). However, we can modify the documentation to not show recommended if the term is already required, that can be its own issue, and this one closed in favor of that issue.

turbomam commented 8 months ago

Good catch @mslarae13 . Good explanation, @sujaypatil96.

I suggest we address this in the schema, not in custom documentation.

I will start a fork of GenomicsStandardsConsortium/mixs and add a Python script in https://github.com/GenomicsStandardsConsortium/mixs/tree/main/src/scripts that reports cases like that and then fixes them. I will make a PR but leave it open for others to review. I propose the use of new forks like this any time somebody wants to test anything that would effect the generation of documentation.

I will base the script on the LinkML SchemaView approach. Hopefully we can turn this into a tutorial for how other people with some some Python skills can examine or improve the schema in bulk.

turbomam commented 8 months ago

In terms of implementation: whenever I see a term is required and recommended in a class' slot_usage, I will remove the recommend assertion.

turbomam commented 8 months ago

I made a stub/draft PR

turbomam commented 8 months ago

Notes to self:

The global definition for depth doesn't assert required or recommended

In Soil the slot_usage for depth says

required: true

In Mims the slot_usage for depth says

recommended: true

and

mims_soil_obj = schema_view.induced_class("MimsSoil")
logging.info(yaml_dumper.dumps(mims_soil_obj))

does report

    required: true
    recommended: true
turbomam commented 8 months ago

@mslarae13 here's a report of the slots usages that are currently both required and recommended

I can easily remove the recommended assertions in those cases, but we should have community discussion.

Are there cases in which it would be useful to assert both required and recommended? Here's the Python code that acts on the legacy MIxS 'Requirement' values.

It has unique solutions for

but C/"conditional mandatory" and E/"environment-dependent" are both being treated as recommended in that code block.

I think the TWG concluded that assigning a slot to an Extension class accounted for the E state and that C was so vague that it couldn't be modeled, but I haven't found a paper trail for those claims yet.

lschriml commented 8 months ago

For MIxS a term is either required or recommended.

On Thu, Oct 19, 2023 at 12:41 PM Mark A. Miller @.***> wrote:

@mslarae13 https://github.com/mslarae13 here's a report of the slots usages that are currently required and recommended

I can easily remove the recommended assertions in those cases, but we should have community discussion.

Are there cases in which it would be useful to assert both required and recommended? Here's the Python code that acts on the legacy MIxS 'Requirement' values https://github.com/GenomicsStandardsConsortium/mixs6.2_release_candidate/blob/f53224abc0debd7124bfd518f70eeed2c0b20a60/src/mixs_envo_struct_knowl_extraction/mixs_linkml_from_xlsx.py#L404-L418 .

It has unique solitions for

— Reply to this email directly, view it on GitHub https://github.com/GenomicsStandardsConsortium/mixs/issues/672#issuecomment-1771354934, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBB4DOOXC5RNDI4DZ6HQSLYAFJ4TAVCNFSM6AAAAAA6GRPEN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZRGM2TIOJTGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Lynn M. Schriml, Ph.D. Associate Professor

Institute for Genome Sciences University of Maryland School of Medicine Department of Epidemiology and Public Health 670 W. Baltimore St., HSFIII, Room 3061 Baltimore, MD 21201 P: 410-706-6776 | F: 410-706-6756 @.***

turbomam commented 8 months ago

For MIxS a term is either required or recommended.

You're not accounting for all of the Requirement values

mslarae13 commented 7 months ago

Thanks for the details @sujaypatil96 . That makes sense. But I am confused as to why depth is in MIMS? That seems weird.. for some of the environments, if you paired with MIMS it wouldn't be applicable. Shouldn't this just be a term on the enviroment?

@only1chunts curious on your thoughts too

only1chunts commented 7 months ago

I believe when the various checklists were created it was felt that Depth is an important value to have whenever its appropriate to be measured, therefore it was added as "Expected" for all checklists. We now use the phrase "Recommended" instead of "Expected" but essentially they have the same meaning. So where Depth is felt to be a "Required" value in a particular extension the "Recommended" assertion in the checklist should be overruled with the "Required" assertion from the extension. Where Depth is not part of the extension then the checklist will still have the Depth term as "Recommended", I'm not convinced that is appropriate because as you point out there are environments where measuring the depth is just not feasible let alone something we should recommend! I feel like we have previously discussed whether all terms should only appear in EITHER checklists OR extensions and not in both, but I cant see a ticket for it, and dont have the bandwidth to go searching meeting minutes right now, I'll add it to the agenda for the next CIG call, so we can get a definite answer and document that somewhere appropriate (where?).