dynverse / dynmethods

A collection of 50+ trajectory inference methods within a common interface 📥📤
https://dynverse.org
Other
118 stars 26 forks source link

p-Creode #43

Closed rcannood closed 6 years ago

rcannood commented 6 years ago

Hello @herrinca and @KenLauLab

This issue is for discussing the wrapper for your trajectory inference method, p-Creode, which we wrapped for our benchmarking study (10.1101/276907). In our dynmethods framework, we collected some meta information about your method, and created a docker wrapper so that all methods can be easily run and compared. The code for this wrapper is located in a docker container. The way this container is structured is described in this vignette.

We are creating this issue to ensure your method is being evaluated in the way it was designed for. The checklist below contains some important questions for you to have a look at.

The most convenient way for you to test and adapt the wrapper is to install dyno, download and modify these files, and run your method on a dataset of interest or one of our synthetic toy datasets. This is further described in this vignette. Once finished, we prefer that you fork the dynmethods repository, make the necessary changes, and send us a pull request. Alternatively, you can also send us the files and we will make the necessary changes.

If you have any further questions or remarks, feel free to reply to this issue.

Kind regards, @zouter and @rcannood

herrinca commented 6 years ago

Thanks for the opportunity to review, will provide feedback shortly.

herrinca commented 6 years ago

I am assuming the comparisons will be ran with the default parameter values?

Also, I was not able to locate a qc worksheet for pcreode.

Thanks

rcannood commented 6 years ago

Hello Charles,

I am assuming the comparisons will be ran with the default parameter values?

Indeed. Are the default values listed in the definition.yml okay for you?

Also, I was not able to locate a qc worksheet for pcreode.

My apologies, this is a mistake on my part. I will have a look at it tomorrow; I'll get back to you asap.

Kind regards, Robrecht

herrinca commented 6 years ago

I am going to write a few simple functions to automate parameter selection on my end, while this is not preferred to supervised parameter selection (I understand a supervised approach is not feasible for comparisons) I believe in our case it is significantly better than using a constant value for each parameter. I hope to have the automated parameter selection pushed to pcreode git by early next week.

rcannood commented 6 years ago

@zouter and I just updated the quality control sheets, so now there is one for p-Creode as well.

Will the upcoming changes to p-Creode result in any changes to run.py?

herrinca commented 6 years ago

Awesome, Thanks.

Yeah, there will be a few changes to the run.py. I plan to make the needed changes and give it a test run or two.

herrinca commented 6 years ago

I have initiated a pull request for updated pcreode files. I have tested it using the sample data set provided in the example, but I would like to test it on other data sets. Are there any other sample sets I can test it on?

I also have a couple notes on the qc worksheet:

30 - We are currently working on a version of pcreode and we are using a branch for this upgrade.

41 - I have added a mute option to all functions that print and I have muted all functions in the run.py file.

49 - Could you please provide an example of quantifying accuracy? Just curious how this has been accomplished.

Thanks and sorry for the delay in getting everything updated. Chuck

rcannood commented 6 years ago

Hi Chuck,

No worries about the timing.

I made a few changes (See PR #105). Are these okay for you?

QC30: I changed the score from 0 to 1. QC41: Good to hear! I changed the score from a 0 to a 1. QC49: If you check row 49 from this google doc, you can find all TI methods with quantitative evaluations coloured in blue :)

Concerning the datasets, @zouter and I should make the next version of our datasets available on zenodo (or something similar). The dyntoy package produces small datasets, but these should not be used for evaluating a TI method as they are far too simplistic. I will notify you when the new version of our datasets have been made easily available.

Kind regards, Robrecht

herrinca commented 6 years ago

Sorry, looks like I missed the noise parameter in the analysis call, instead of

analysis = pcreode.Analysis( file_path = "/ti/workspace/.", graph_id = gid, data = pca_reduced_data, density = density, noise = params["noise"] )

it should be

analysis = pcreode.Analysis( file_path = "/ti/workspace/.", graph_id = gid, data = pca_reduced_data, density = density, noise = noise )

That should get rid of the need for a noise parameter.

rcannood commented 6 years ago

No problem!

The container code is now located at https://github.com/dynverse/ti_pcreode/blob/master/run.py.

If you have any further comments or questions, feel free to create a new issue there!

Robrecht