Inconsistencies Between Code, Documentation, and Schema

I went through the code in the "model" folder, the JSON Schemas in "resources/schemas/isa_model_version_1_0_schemas", the JSON Schemas on https://isa-specs.readthedocs.io/en/latest/isajson.html, and the specification on https://isa-specs.readthedocs.io/en/latest/isatab.html to try and find inconsistencies. My impetus to do this was after trying to figure out the correct way to fix one of the validation functions and realizing that I don't have a source of truth for what should be valid.

I am going to list off some of what I found and categorize them a little bit.

Hard Inconsistencies

1. The JSON Schemas actually used by the code in "resources/schemas/isa_model_version_1_0_schemas" are different from what is in the documentation at https://isa-specs.readthedocs.io/en/latest/isajson.html. The differences are slight, but the documentation should match what is actually in use.
2. In the "to_dict" method for the Process class, "comments" for parameterValues are not added to the dictionary created for them. Assuming "to_dict" is how you are supposed to create a JSON version from an Investigation instance, and since there is no where else in the JSON representation that these can appear, this should probably change to include the comments. I did not check if these comments appear in Tab.
3. The "to_dict" method for the Protocol class just puts an empty list for "components". The Protocol class also has "components" as a list of OntologyAnnotations, but this does not match what is in the JSON Schema in the "resources/schemas/isa_model_version_1_0_schemas" folder.
4. The Material class doesn't have "derivesFrom" anywhere, this does not match what is in the JSON Schema.
5. The Characteristic class has the "category" as an OntologyAnnotation, but this does not match the JSON Schema. The StudyAssayMixin class also has "characteristic_categories" as OntologyAnnotations which does not match the JSON Schema, but the "categories_to_dict" method puts them in the form that matches JSON Schema. The Characteristic class's "to_dict" method does not put them in the form that matched the JSON Schema though.
6. The FactorValue class does not put "comments" in the dict created by the "to_dict" method, but the class is Commentable.

Minor Things I Noticed

1. The schemas in "resources/schemas/isa_model_version_1_0_schemas" include an "organization_schema.json" file that isn't used by any of the schemas in that folder.
2. The JSON-LD attributes aren't able to be set using "from_dict" for most classes, like Person and Publication. You can set "@id" for OntologyAnnotation, but not the other LD attributes. These are just some examples.

Questions

1. Why do some nodes/classes not inherit from Identifiable? For instance, the Publication class doesn't. It makes it so you can't set an "id" and multiple calls to "to_dict" will generate a new "@id" property each time, so it isn't persistent.
2. There are multiple folders for JSON Schemas in "resources/schemas". In particular, folder "isa_model_version_2_0_schemas" seems like it might be a version 2 based on the name. Are there plans to revamp the JSON representation of ISA? The schemas in "isa_model_version_2_0_schemas" are quite different from what are in "isa_model_version_1_0_schemas".

Suggestions or Possible Improvements

1. For processes it might be a good idea to allow the "performer" attribute to be a person_schema in addition to a string, so people can put an "@id" there if they want. I actually assumed this had to be a person_schema because that would match the pattern used in so many other attributes.

The Hard Inconsistencies between the model code and JSON Schemas really needs addressed, and most importantly, which one is the ground truth? Does the model code need to change or the JSON Schemas?

Deprecated Columns?

This next bit I want to do separately because I really need to understand it to fix the validation function that started this whole thing. The function in question is "load_table_checks" in "isatab/validate/rules/rules_40xx.py". It is printing an unexpected column error for a "Characteristic" column after an "Extract Name" column. That is to say the function is saying that it is not valid to have a "Characteristic" column after an "Extract Name" column. I am quite confident that this is not correct. When I went to double check the truth of this I ran into what seems like contradictory information from my point of view between some of the sources mentioned above. That's why I did a deeper dive into things to try and find a decent list of issues that we can hopefully get resolved so there isn't conflicting information and code.

The main thing I want to ask about now is in the specification here: https://isa-specs.readthedocs.io/en/latest/isatab.html#assay-table-file. Under "2.3.7 Assay Table file" where it talks about "Extract Name" and "Labeled Extract Name" it says they "MAY be qualified with Characteristics, Material Type and Description". The "Characteristics" is consistent with the JSON Schema and model code, but I don't think "Material Type" and "Description" are relevant anymore. At the very least I cannot see what "Material Type" and "Description" would translate to in the JSON or model. I think maybe "Material Type" would just be a characteristic, and "Description" would just be a comment.

I have looked over some of the conversion code and loading code, and I can find where there is some special handling for "Material Type" in the "load_table" function in the "isatab/load/core.py" file, and the first version of the tab2json converter, "ISATab2ISAjson_v1", also has special handling for "Material Type", but I can't find anything for "Description". It should be noted that this "load_table" function is only used for the ISA-Tab validator, the converter, new and old, load tables in a different way. The way the new converter loads tables does not have any special handling for "Material Type" or "Description". All of the special handling for "Material Type" makes it into a characteristic.

Similarly, "Label" has the same issues as "Material Type" and "Description". To me it looks like it should just be a characteristic. I can find special handling for "Label" in the old tab2json converter and in the new converter (ProcessSequenceFactory). Both turn it into a characteristic.

Can we decide exactly what should be done with these 3 columns, "Label", "Material Type", and "Description"? The easiest thing is to just drop them and remove their mention in the documentation, but if you want to stay backward compatible then they need some special handling. "Label" and "Material Type" already have some special handling precedent that simply turn them into characteristics, which I think is fine, but needs to be consistent everywhere in the code. The 3 different loading paradigms in the code all do things differently. "Description" has no precedent for special handling that I can find, so it might be okay to just drop this and remove all mentions. I do need to have an answer to this before I can fix the "load_table_checks" function though.

This last part may need to be it's own issue, but it is related to everything. If you would rather have it separated, then that's fine.