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

Add specimen and taxon circumscription #75

Merged
merged 14 commits into from
Mar 9, 2021

Conversation

gaurav
Copy link
Member

@gaurav gaurav commented Feb 16, 2021

This PR incorporates two phyloreferences from Fisher et al, 2007, which include examples of specimens and a taxon circumscribed by a publication, as well as one of the phylogenies from that paper for testing. In doing this, I found a bug with how phylorefs with external specifiers were being generated when there are multiple external specifiers, and rewrote the algorithm to eliminate this bug. I also added a example file example_five_external_specifiers.json, which has a phyloreference with five external specifiers, just to make sure those logical expressions are being generated correctly.

It also includes a minor fix to error reporting in phyx2owl.js.

Should be merged after PR #79 and PR #72.

@gaurav gaurav force-pushed the add-specimen-and-taxon-circumscription branch from 7f479d4 to 660f531 Compare February 24, 2021 04:39
@gaurav gaurav changed the base branch from master to add-apomorphy-phyloref February 24, 2021 04:42
@gaurav gaurav force-pushed the add-specimen-and-taxon-circumscription branch from 6676b5f to 48a6d0c Compare March 2, 2021 22:37
@gaurav gaurav changed the base branch from add-apomorphy-phyloref to test-nomenclatural-code-fallbacks March 2, 2021 22:37
@hlapp
Copy link
Member

hlapp commented Mar 5, 2021

@gaurav did you forget to mark this one ready for review, or did you find you have to return it to draft stage temporarily?

@gaurav gaurav marked this pull request as ready for review March 7, 2021 21:04
@gaurav
Copy link
Member Author

gaurav commented Mar 7, 2021

@hlapp Oops, yes, I forgot to mark it ready for review! My bad.

Copy link
Member

@hlapp hlapp left a comment

Choose a reason for hiding this comment

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

Look OK. See inline comment. And you still haven't explained to me (or maybe you have and I don't remember where?) why we are using the development version of the Phyx context.

remainingExternals.forEach((externalTU) => {
intersectionExprs.push({
'@type': 'owl:Restriction',
onProperty: 'obo:CDAO_0000144', // has_Ancestor
Copy link
Member

Choose a reason for hiding this comment

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

Note that there's a property has_outside_TU for this in the next release of the Phyloref Ontology: https://github.com/phyloref/phyloref-ontology/blob/master/phyloref.ofn#L38

Not sure whether using it should be a downstream change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I forgot about that property! I've opened an issue to make the change in Phyx.js once the new version of the Phyloref Ontology has been released: #83.

@gaurav
Copy link
Member Author

gaurav commented Mar 8, 2021

Look OK. See inline comment.

Thanks, Hilmar!

And you still haven't explained to me (or maybe you have and I don't remember where?) why we are using the development version of the Phyx context.

I tried to answer this question in #72 (comment) after you asked it here, but if you'd like me to document this in a README file in the test/ directory, let me know!

@hlapp
Copy link
Member

hlapp commented Mar 8, 2021

And you still haven't explained to me (or maybe you have and I don't remember where?) why we are using the development version of the Phyx context.

I tried to answer this question in #72 (comment) after you asked it here, but if you'd like me to document this in a README file in the test/ directory, let me know!

I think it would indeed be good to have that documented somewhere. That the question has already come up repeatedly indicates that before too long it will again, and so it seems useful to write this down so we don't have to dig around for the answer.

Base automatically changed from test-nomenclatural-code-fallbacks to master March 9, 2021 16:23
@gaurav gaurav merged commit e795b11 into master Mar 9, 2021
@gaurav gaurav deleted the add-specimen-and-taxon-circumscription branch March 9, 2021 16:24
@gaurav
Copy link
Member Author

gaurav commented Mar 9, 2021

I think it would indeed be good to have that documented somewhere. That the question has already come up repeatedly indicates that before too long it will again, and so it seems useful to write this down so we don't have to dig around for the answer.

Sounds good! I've written them up in PR #84.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants