Initial implementation of a stand-alone phenotype similarity scoring service#259
Open
allasm wants to merge 1 commit into
Open
Initial implementation of a stand-alone phenotype similarity scoring service#259allasm wants to merge 1 commit into
allasm wants to merge 1 commit into
Conversation
…service. - service is @ /rest/patients/matching-notification/compute-score - service accepts POST requests and has two POST parameters, `referencePatient` and `matchPatient` (both should be JSON strings in the Phenotips JSON format) - `referencePatient` and `matchPatient` are interchangeable - service only scores phenotypes, disorders and genes are ignored
sdumitriu
requested changes
Mar 21, 2019
| * @return result JSON | ||
| */ | ||
| @POST | ||
| @Consumes(MediaType.APPLICATION_JSON) |
Member
There was a problem hiding this comment.
This doesn't really consume json, since it manually parses jsons from an x-www-form-encoded list of parameters.
| this.slf4Jlogger.error("Can't convert JSON to a patient: [{}]", ex.getMessage()); | ||
| return null; | ||
| } | ||
| final Patient patient = this.repository.create(); |
Member
There was a problem hiding this comment.
This actually creates a patient, so it is not suitable for use in production.
Some alternatives:
- Make sure you delete the patient at the end (preferably skipping the document trash), although this will create some churn
- Although using internal classes is discouraged, in this case it would be better to manually create a PhenoTipsPatient on a new XWikiDocument in a temporary space, and later delete that document
- Implement the
Patientinterface like it's done inorg.phenotips.data.script.PatientSpecificityScriptService.FakePatient, with no actual persistence and only implementing the methods the score cares about - Make that an actual public implementation,
TransientPatientImplthat other services can use for temporary storage; it could extendPhenoTipsPatientand override methods so that they never actually save the underlyingXWikiDocument, but otherwise it would behave just like a real patient
|
|
||
| <properties> | ||
| <coverage.instructionRatio>0.59</coverage.instructionRatio> | ||
| <coverage.instructionRatio>0.54</coverage.instructionRatio> |
| import javax.inject.Singleton; | ||
|
|
||
| /** | ||
| * For Arun. |
| public interface PatientSimilarityScorer | ||
| { | ||
| /** | ||
| * Returns phenotype similarity score. |
Member
There was a problem hiding this comment.
The name of the class suggests that this is a generic similarity scorer, not one that only looks at phenotypes. Either:
- keep the API to return just a single score, but then modify the javadoc to be more generic, and write different implementations for different score aspects
- modify the API to return more than one score, possibly as a map (
{"phenotype":0.6, "genotype": 1.0, "disease":0,"overall":0.8})
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not merge, this PR is for convenience of monitoring changes only