OpenTreeOfLife / phylesystem-api

API access to Open Tree of Life treestore
BSD 2-Clause "Simplified" License
9 stars 5 forks source link

NexSON syntax for tree synthesis status #191

Open kcranston opened 7 years ago

kcranston commented 7 years ago

On opentree issue#1042 we have been finalizing plans for changing how users propose trees for synthesis. We (= primarily me and @jimallman ) now need to determine now we are going to represent these new properties in the NexSON. Here is a proposal for discussion:

  1. Remove the study-level ot:candidateTreeForSynthesis property that currently holds a list of treeIds where Preferred = True
  2. Add a tree-level property called ot:candidateForSynthesis with the following possible values: [Not reviewed,Do not use,Include,Needs curation]. The values correspond 1:1 to menu choices in the UI.

Q: In the UI, the default value is Not reviewed. Would we add the property : value {ot:candidateForSynthesis:Not reviewed} to all of the NexSONS? Or simply assume that as the default value if the property does not exist?

jimallman commented 7 years ago

Q: In the UI, the default value is Not reviewed. Would we add the property : value {ot:candidateForSynthesis:Not reviewed} to all of the NexSONS? Or simply assume that as the default value if the property does not exist?

The curation app typically does the latter, adding expected properties (w/ default values) to a study as it's opened.

Given the changes outlined above, I believe I can similarly interpret any tree IDs found in the study-level ot:candidateTreeForSynthesis as ot:candidateForSynthesis: 'Include' in the listed trees. This should avoid the need for a "zero day" with simultaneous changes to the main docstore and the curation app.

kcranston commented 7 years ago

Sounds good. Are there any validation issues with phylesytem-api that we need to worry about?

jimallman commented 7 years ago

Are there any validation issues with phylesytem-api that we need to worry about?

Perhaps this bit of ws-tests/test_study_get_multiformat.py that looks at the ^ot:candidateTreeForSynthesis property. Otherwise I think we're clear.

mtholder commented 7 years ago

Ideally, if a doc passes the checks in $PEYOTL_ROOT/scripts/nexson/validate_ot_nexson.py then the API should accept it too (w/ the exception of size constraints that are imposed by the API). I can't recall how we deal with new properties during validation. Some tweaks to $PEYOTL_ROOT/peyotl/nexson_validation/schema.py may be needed.

jar398 commented 7 years ago

This is a little controlled vocabulary and it would be good to use qnames, as one normally would for a controlled vocabulary, e.g. ot:notReviewed, ot:doNotUse / ot:use (or ot:doNotInclude / ot:include), ot:needsCuration

kcranston commented 7 years ago

I like that suggestion: ot:notReviewed, ot:doNotInclude, ot:include, ot:needsCuration

edited for lower camel

jar398 commented 7 years ago

(except lower camel ot:include, not ot:Include)

jimallman commented 7 years ago

@kcranston Are you OK with ot:include, for consistent camel-case?

kcranston commented 7 years ago

I already edited my comment for consistency of camels

jimallman commented 7 years ago

Just an update to note the latest proposal, which adds a list of objections (reasons to exclude) for each tree in Nexson, for example:

...
"ot:reasonsToExcludeFromSynthesis": [
  "Felis as root conflicts with generally accepted phylogeny.",
  "Needs further curation, esp. OTU mapping"
], 
...

Proposed enhancement: If we want to preserve "resolved" objections, we'll need one more layer (using $ for the main string, Badgerfish style):

...
"ot:reasonsToExcludeFromSynthesis": [
  {
    "$": "Felis as root conflicts with generally accepted phylogeny.",
    "ot:resolved": true
  },
  {
    "$": "Needs further curation, esp. OTU mapping",
    "ot:resolved": false
  },
], 
...

This proposal also removes any properties indicating the tree's current state (presence in synthesis collections) or our intentions for next synthesis. These are handled by the use of Include and Exclude buttons in the curation app, which will take effect immediately using the collections API to add/remove the current tree as needed. When this method responds, we update the tree-list display to reflect whether the tree is now in a synthesis collection (ie, one whose members will be included in synthesis by default).

Comments welcome. Barring objections, I'll add this to the main Nexson documentation (wiki page).

kcranston commented 7 years ago

How do objections get resolved?

jimallman commented 7 years ago

This could be handled similarly to comments in a Google doc, which offer a Resolve button (perhaps as an alternative to simply deleting the objection).

jar398 commented 7 years ago

Will the NeXML round trip still work with lists like this?​

kcranston commented 7 years ago

I think I need to see a sketch of the proposed UI before giving a 👍

jimallman commented 7 years ago

Will the NeXML round trip still work with lists like this?​

I believe so -- at least, I can't think of any reason why not.

I think I need to see a sketch of the proposed UI

Of course. I'm hoping to knock out some mockups later today.

mtholder commented 7 years ago

It seems to me like we should just start with a list of reason and add complexity later if we need it. Git provides us with history info for these fields, so I'm not convinced we need to hold the history in the doc (with the resolved=true being a form of recording the history).

jimallman commented 7 years ago

Git provides us with history info for these fields

Good point -- I'll omit this wrinkle for now, since we can add it later if users wish they could see prior deliberations.

jimallman commented 7 years ago

I've updated the Nexson wiki page to include ot:reasonsToExcludeFromSynthesis, and marked ot:candidateTreeForSynthesis and ot:notIntendedForSynthesis as deprecated.

jimallman commented 7 years ago

Here's the mockup so far, still working out some details:

This is based closely on @kcranston's Balsamiq mockup, with some implementation details added.

screen shot 2016-10-19 at 3 34 56 pm

Some questions and observations:

mtholder commented 7 years ago

I think that this looks quit nice. I agree that "include in synthesis?" is better than "Action"

I do think the badges look nice, and more informative than "details"

jimallman commented 7 years ago

Here's a proposed design for the resulting "details" popup:

screen shot 2016-10-19 at 11 46 26 pm

It tries to explain all possible factors in an accessible way. A few implementation details:

kcranston commented 7 years ago

This all looks good to me! Thanks! (and agree about changing the heading on the 'Action' column)