-
Notifications
You must be signed in to change notification settings - Fork 15
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
Ethology module: Upper level changes by @DitchingIt #189
Conversation
Unfortunately the process removes links to child classes, unless those children have their own line in the template defining their parents. |
@matentzn Looks good. Do I need to do anything here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the new classes inserted below NBO_0000313, despite the slight increase in complexity. I agree that splitting parental and reproductive behavior is controversial, but combining them might break existing inferences, so let's stick with that for now.
Line 1220 - an extra space at the start of the synonym string
Line 1865 - same
Maybe add 'playing behavior' (US spelling) as a synonym
Line 2033 - What's the mess at the end of the definition; guessing it's a TODO that needs attention.
I'll leave this open for @dosumis to review. Maybe @aclark-binghamton-edu should look at the fear and reproductive behavior changes as well.
Line 2033
@pmidford Thanks:
Here is the modified ROBOT sheet and I suggest that merging the next PR be the next step (barring odd things like item 3 above) if there are no more comments added in the next week. |
Taking a look at this and I have a few initial thoughts/questions, in no particular order:
|
@sbello 's points 1 and 2 remind me of earlier discussions stemming from ABO which attempted to separate more strictly observation from interpretation. The suggested compromise was that we allow for defined terms combining observation and interpretation - so we might have some more explicit compound terms like "fear related behaviour in response to external threat" composed of separate terms for 'fear related behaviour' and 'response to external threat'. |
Thanks @sbello for your really useful comments. I would welcome more in future as @dosumis suggests. I can only take a moment now so I will just answer your first point. Is it correct to use 'external threat' for all fear related behavior? Fear responses can be triggered by things that aren't actual threats but only perceived threats. Does that fall within the definition?
More later and again thanks. |
COMMENT 2: Are the terms placed under survival behavior ALWAYS related to survival? If not these should not be placed under this term. I am indebted to @sbello for taking the bull by the horns. My instinct, without good ontological experience, is to aggregate inclusively rather than exclusively. I now see that I have made a mistake, and I'm glad to have it pointed out early enough to take account of it in future. As for 'survival behavior', if it can't be a simple aggregator, this isn't the place to introduce it as a new term. I agree with @dosumis about using combined terms, but I don't have the time or energy to put 'survival behavior' out on its own limb and connect it back in where appropriate, although it is something to put on a list for later. For now, I will remove it from the structure and simply connect its children directly to 'behavior process'. I won't be able to do this for a few days so don't hold your breath. I'll look at comments 3 and 4 tomorrow. Thanks |
COMMENT 3: I have in my notes for MP that we can only have 1 comment for a term as the presence of multiple comments is not allowed in the OBO format version of the file. Assuming this is still true the multiple comments need to be combined into a single comment.
|
COMMENT 4 I had taken the issue of preserving meaning when changing definitions or labels to heart, but thanks for the reminder. For example the original way that NBO framed reproduction excluded brooding and parenting. I have preserved this despite many references including them. This is something to keep an eye on in future. |
I have uploaded an updated Google sheet with the following changes:
|
Thanks for the latest commit @matentzn. Unfortunately it retains the old links to NBO:0070005 'survival behavior' which was removed; specifically from NBO:0000091, NBO:0070007, NBO:0070008, NBO:0000338, NBO:0000011 and NBO:0070006. |
Are you certain? The pipeline
|
I'm ready to post a review and let this out, but what are these failing qc checks? Are they cause for concern? |
@matentzn @pmidford @sbello I have uploaded a slightly edited ROBOT template (to the same location as before) with a number of redundant comments removed, and added a subset to more easily search for the parts of NBO that this effort is editing or adding. I don't know if the qc failure is related to the construction of this pipeline? It was our first time and Nico has included 'never merge' in the title??? |
@matentzn When you have time, the pull request is titled 'DRAFT (never merge)...' and the QC checks have from the beginning said 'Error: Process completed with exit code 2'. @pmidford and I are wary of completing our reviews but would like to merge the changes. Your thoughts would be gratefully received. |
The checks have passed now (I made a change to you template in the "subset" column) but this PR needs a positive approval from @pmidford to be merged in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, absent any further comments from @sbello and since other issues have been addressed, I approve this merge.
@DitchingIt which issues does this address, can you provide a three sentence high level summary of the changes, and tell me a more appropriate title for the pull request? |
Ethology module: Upper level changesAddressing #184, these changes update five of the highest levels of the NBO's process branch and add three new ones to enable future reaggregation of a number of its sub-branches. Two labels are recast, definitions follow a standard NBO style, and fixed sources are given. Annotation properties closely follow OBO Foundry recommendations. |
Addressing #184, these changes update five of the highest levels of the NBO's process branch and add three new ones to enable future reaggregation of a number of its sub-branches. Two labels are recast, definitions follow a standard NBO style, and fixed sources are given. Annotation properties closely follow OBO Foundry recommendations.
This PR was opened by me, but performed by @DitchingIt