OHDSI / ROhdsiWebApi

An R package for interfacing with a WebAPI instance
https://ohdsi.github.io/ROhdsiWebApi
10 stars 17 forks source link

Develop post definitions to web api #120

Closed gowthamrao closed 4 years ago

gowthamrao commented 4 years ago

https://github.com/OHDSI/ROhdsiWebApi/issues/106

Adds ability to post concept set and cohorts. We have to discuss how to post incidence rate, estimation, prediction, pathway and characterization

codecov-commenter commented 4 years ago

Codecov Report

Merging #120 into develop will decrease coverage by 30.04%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #120       +/-   ##
===========================================
- Coverage    30.04%   0.00%   -30.05%     
===========================================
  Files           10      10               
  Lines         1145    1327      +182     
===========================================
- Hits           344       0      -344     
- Misses         801    1327      +526     
Impacted Files Coverage Δ
R/AutoGeneratedDefinitions.R 0.00% <0.00%> (-19.85%) :arrow_down:
R/CohortDefinition.R 0.00% <ø> (-25.17%) :arrow_down:
R/Deprecated.R 0.00% <0.00%> (ø)
R/Private.R 0.00% <0.00%> (-10.72%) :arrow_down:
R/WebApi.R 0.00% <0.00%> (-77.62%) :arrow_down:
R/Concept.R 0.00% <0.00%> (-100.00%) :arrow_down:
R/ConceptSet.R 0.00% <0.00%> (-71.43%) :arrow_down:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 98ee8ff...ceb1d9c. Read the comment docs.

schuemie commented 4 years ago

This pull request also includes all edits from the previous two pull-requests, which are still being discussed. So I'll not merge this one for now.

Just curious: why don't we apply the same pattern here that is used everywhere else? So have postCohortDefinition, postConceptSetDefinition, etc.

What did you want to discuss about posting incidence rate, estimation, prediction, pathway and characterization?

Just a mental note for myself: I can think of two main use cases here:

  1. People will want to pull definitions, edit them programmatically in R, and then post them back. (e.g. I want to create several exposure cohorts based on one, by swapping out the exposure concept set).

  2. People will want to pull definitions from one ATLAS, and insert them into another.

gowthamrao commented 4 years ago

Just curious: why don't we apply the same pattern here that is used everywhere else?

I was finding it hard to find a systematic pattern for POST. e.g. there seemed to be a two step process for concept set expression. First step - do a POST of metadata without expression to get a concept set id. Then do PUT with that id to add the expression. vs. POST the expression in cohort.

What did you want to discuss about posting incidence rate, estimation, prediction, pathway and characterization?

I have been deducting the logic by experimenting here. Instead, i think a brain storming session with @chrisknoll and @anthonysena would be very helpful to understand the pattern. If we can get a reference for each category, then i think we can programmatically generate them - maybe capture it in this https://github.com/OHDSI/ROhdsiWebApi/blob/287cb55d777948e4675daf34f308a1ed77c56cae/R/Private.R#L32

schuemie commented 4 years ago

I was finding it hard to find a systematic pattern for POST. e.g. there seemed to be a two step process for concept set expression. First step - do a POST of metadata without expression to get a concept set id. Then do PUT with that id to add the expression. vs. POST the expression in cohort.

Ok, but how is that a problem specifically of that pattern, but not using a single function?

schuemie commented 4 years ago

Looks good, except there's all the old code from other pull requests (including 'checkIf...'. Would you mind recreating this pull request? Maybe merge the current develop into your own develop-postDefinitionsToWebApi branch.

chrisknoll commented 4 years ago

I'm fine to have a design session with you guys on this, I think from a 'pattern' perspective, we can follow our typical CRUD model for dealing with these types of entities (create-read-update-delete), and almost all of our WebAPI calls support the notion of a single operation function to invoke the operation (the notable exception of concept sets). Just consider the design around concept set expressions a mistake and shouldn't be considered in your overall API design...in all other cases, you'll be able to have a R API function 1-to-1 with a webAPI end point. In the case of concept set expressions, I'd suggest 'wrapping' the multiple function calls for the 'save' operation into a single R function, but expose the R api as a single 'saveConceptSet()'.

Also, on the note of post/put/get in the function API, i wouldn't do that..the post/put stuff is just HTTP verbage that isn't important to the purpose of the function...i think createXXX, saveXXX, deleteXXX, getXXX should be the only functions you need. (from a CRUD perspective, the other ones like 'generate' are other API functions that should be exposed.