Add license information

theislab / scib-reproducibility

Additional code and analysis from the single-cell integration benchmarking project

https://theislab.github.io/scib-reproducibility/

MIT License

52 stars 14 forks source link

Add license information #21

Closed rcannood closed 1 year ago

rcannood commented 1 year ago

Thanks to openproblems-bio/openproblems#689 I became aware of the existence of the visualization/knit_table.R source code. After inspecting it,I can only conclude that the knit_table.R is in violation of the dynbenchmark LICENSE, which stipulates that the license should be included in copies of the software, even if they have been modified afterwards.

I also don't appreciate copying our source code and passing it off as your own by calling it scIB_knit_table().

This PR adds the license information and a reference to funkyheatmap, which I have since created to allow other people to generates plots like these.

LuckyMD commented 1 year ago

Hi @rcannood,

Thanks a lot for flagging this. We cited and attributed that the visualization was adapted from dynbenchmark in the scib paper, but it looks like we completely missed this in the repo, which is not okay. Next to the above (license, code documentation), this should be documented in the README of the visualization folder as well.

As far as I'm aware @martaint tried to use the dynbenchmark visualization code, but ended up rewriting a lot as it was challenging to adapt to our setting. This is how I interpret the scIB_knit_table.R renaming.

rcannood commented 1 year ago

Is this PR waiting for further changes? :)

LuckyMD commented 1 year ago

I would like comments here from @lazappi and ideally @martaint as they wrote/reviewed this code before merging.

rcannood commented 1 year ago

Thanks for the feedback @LuckyMD @lazappi! I apologise for my harsh tone in the original post. It was a lot ruder than it needed to be.

I have it some more thought and have been wondering about the following.

I saw that scIB_knit_table() contains code for plotting images and arrows. dynbenchmark::funky_heatmap() also supported something like this, but it was hardcoded and only for plotting trajectory types.

I think it would make sense to incorporate arrows (maybe) and images as a geom to funkyheatmap::funky_heatmap(). I think it then would also make sense to add you all as contributors to the package.
I included the dynbenchmark plot as an example in one of the vignettes of funkyheatmap. I would like to reproduce fig. 2 of the scIB paper as best as possible and include it as a separate vignette (will need to add some options to funky_heatmap()). Would that be ok with you all?

rcannood commented 1 year ago

Can somebody (@lazappi @LuckyMD @martaint @mumichae) merge this PR? ;)

@LuckyMD is the only one who didn't explicitly approve this PR, but I feel I adequately addressed his comments, and I don't think that he has a lot of time to look through GitHub PRs at the moment :relaxed:

LuckyMD commented 1 year ago

done. Thanks for doing this, Robrecht! Really excited to get funkyheatmap published :). I believe @martaint was also interested in helping.