search

nadeemlab / SPT

Spatial profiling toolbox for spatial characterization of tumor immune microenvironment in multiplex images
https://oncopathtk.org
Other
21 stars 2 forks source link

Convert `cg-gnn`/`histocartography` to a git submodule #269

Closed CarlinLiao closed 11 months ago

CarlinLiao commented 11 months ago

As I mentioned in another issue, we can use git submodules to allow us to both incorporate cg-gnn into SPT as we wanted to do in #257 while also maintaining traceability to the source repositories it was cloned from.

In addition to adding this behavior, this would also establish a pattern that we can use to incorporate alternative graph deep learning models, as we described as wanting in #268.

jimmymathews commented 11 months ago

This is sort of a wacky feature, is there a definitive benefit? A great strength of git is its simplicity in many aspects, including the single repository-level directory concept.

jimmymathews commented 11 months ago

Every software has modules, I'm not sure why we would introduce artificial coupling of the module definitions with git controls.

CarlinLiao commented 11 months ago

This is still a standard git feature, surprisingly. A lot of software we want to use is not available in package managers, e.g., research code like most alternative graph neural network architectures. This gives us a way to use them that is fully version controlled under our jurisdiction. This is, in my opinion, the best way to build out functionality for alternative deep learning architectures, as the alternatives that I see are either copying their code wholesale into our repository, losing direct provenance along the way, or instructing users to clone those repositories on their hardware manually, in which case it's more difficult to ensure that they're using the same version/commit that we built and tested for (part of what git submodule saves to history is the exact commit we're using, which can be updated at any time).

CarlinLiao commented 11 months ago

It might also be worth renaming the cggnn submodule to (cell-)graphs or something because I find it confusing when developing that we have an SPT feature and a GNN model architecture that're the name thing, and it would better reflect the scope creep as we move toward offering multiple deep learning model architectures.

jimmymathews commented 11 months ago

I remain skeptical of the value of git submodules in general and specifically for this usage. This issue could be re-opened if there is new information, of course.

CarlinLiao commented 11 months ago

This issue is clobbered by #272 too. The PR should be rejected as well.