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

Improvements to EntitySetSlimmer #216

Merged

Conversation

deepakunni3
Copy link
Member

  • Use enum to specify available category
  • Handle multiple subject IDs as input

Use enum to specify available category
Handle multiple subject IDs as input
@deepakunni3
Copy link
Member Author

@nathandunn FYI

@nathandunn
Copy link
Contributor

Sure, we can add expression as needed to support the AGR use-case (or they can issue a PR)

@nathandunn
Copy link
Contributor

Actually, I think that having them issue a PR is the better way to go here.

Copy link
Member

@cmungall cmungall left a comment

Choose a reason for hiding this comment

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

Remember there will be other categories in future, but this is fine for now

@deepakunni3
Copy link
Member Author

Yes, making a note to add additional categories to the enum when they are supported.

@deepakunni3 deepakunni3 merged commit c8250d3 into monarch-initiative:master Aug 27, 2018
@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

So it's unclear exactly how this changes the URL that is used for the query. Would you spell it out please.

@nathandunn
Copy link
Contributor

@selewis I think the URL will be identical. I think this just enumerates the categories explicitly.

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

huh?
They already look different.
https://api.monarchinitiative.org/api/bioentityset/slimmer/phenotype?
vs.
https://api.monarchinitiative.org/api/bioentityset/slimmer/function?
vs.
whatever we need for some other ontology...

@nathandunn
Copy link
Contributor

nathandunn commented Aug 28, 2018 via email

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Sorry, still isn't clear (and not sure why we'd let the MODs be the deciders, opinions sure).
What would each URLs be for GO, HPO, and UBERON exactly?

@nathandunn
Copy link
Contributor

Everything is pretty much the same. We were just specifying what we were providing AFAIK.

But its a collaborative process and this is something we all agreed on in the API call.

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Could you just answer the question please. "Pretty much the same" doesn't give me any information.

What would each URLs be for GO, HPO, and UBERON exactly?

@nathandunn
Copy link
Contributor

The URL has not changed at all AFAIK. They were just specified in the spec. So the GO slim, HPO slim, uberon slim, etc. should be what you had before and will not have changed AFAIK. Is this correct @deepakunni3 ?

@deepakunni3
Copy link
Member Author

@selewis @nathandunn The URLs haven't changed at all.

This PR pretty much shows what values are possible for the URL parameter category in the Swagger UI. It does not change the way the endpoint behaves.

@selewis To answer your question, following are the URLs:

GO - https://api.monarchinitiative.org/api/bioentityset/slimmer/function
HPO - https://api.monarchinitiative.org/api/bioentityset/slimmer/phenotype
UBERON - https://api.monarchinitiative.org/api/bioentityset/slimmer/expression (yet to be implemented)

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Except there wasn't anything for UBERON, that's the one that was missing. Maybe it's there now, but we've lost some clarity here. If I look at the swagger page I can see "category * string", but nothing about what are the allowed values. And how does this change the URL? before the URL included "function" for GO or "phenotype" for HPO. Is that gone now? Is it now category="xxx" ?

Someone please spell this out - and ah! Deepak just did...

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Note that the swagger presentation doesn't list any of these.

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Let me know please when expression is ready.

@deepakunni3
Copy link
Member Author

@selewis Since this PR was merged yesterday, the latest changes are accessible at:
https://api-dev.monarchinitiative.org/api

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Yes, saw that, but as I said this currently lacks any information on these options.

@deepakunni3
Copy link
Member Author

Oh, I see. Yes, I can describe the category parameter with more information

@selewis
Copy link
Contributor

selewis commented Aug 28, 2018

Thanks, that will help.

@deepakunni3 - also question on "expression", to be comprehensive it would meed to have slim classes from both Uberon and GO cellular component. Could make two calls (one on 'function' and one on 'anatomy') or one call (with 'expression' looking over both ontologies). What do you prefer?

@deepakunni3
Copy link
Member Author

I think one call looking over both ontologies would be ideal

@selewis
Copy link
Contributor

selewis commented Aug 29, 2018

Certainly would be more convenient from the client side.

@deepakunni3
Copy link
Member Author

deepakunni3 commented Aug 29, 2018


~~~@cmungall What would you recommend?~~~~

Please disregard this.

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.

4 participants