Closed riccardomurri closed 7 years ago
Of course, this PR is too large to review: I guess you'll have to take my word that it makes sense...
Makes sense to me. @sparkvilla When the tests are running, can you test that pull request?
@jluethi I can test this branch, however autotest on pull request can be run once we merge TM in a unique repo, please see on trello. Otherwise it can be "tested" it on a vm.
@riccardomurri @jluethi I have tested this feature branch on a vm with a test dataset. The workflow goes through but the available modules are not showing up
The workflow goes through but the available modules are not showing up
This seems to require a change in how the TissueMAPS/TmLibrary code discovers modules to run. Unfortunately, the current code assumes that all modules reside in a single directory (modules_home
). While changing that to be a list of directories is straightforward, the same configuration parameter is used to locate the handles/
directory -- so we must disentangle that as well...
Should not be a big deal, but definitely not the 10-minute patch that I was thinking of. :-/
We could have modules_home
pointing to the root folder (and assume that it contains a modules
and a handles
subfolder)
We could have modules_home pointing to the root folder (and assume that it contains a modules and a handles subfolder)
That's not how the modules files are laid out in this pull request...
And I would take the occasion to make the mechanism a bit more general so we can support multiple module directories (e.g., I can add a directory for locally-developed modules, or 3rd-party modules, etc.)
Running code with this new layout requires TmLibrary modifications from https://github.com/TissueMAPS/TmLibrary/pull/45
@jluethi @sparkvilla I have tested this PR and TissueMAPS/TmLibrary#45 with the "v0.3.3/CellVoyager_2D_Canonical" test data set and they work. (Both need to be applied to work -- they cannot work separately.)
@riccardomurri This branch off master. Can we merge this first into develop?
This branch off master. Can we merge this first into develop?
Well, master
and develop
are currently at the same HEAD commit::
$ git rev-parse origin/develop
f691151474605e4b4c04d6b523fb43495fc0b813
$ git rev-parse origin/master
f691151474605e4b4c04d6b523fb43495fc0b813
so it would not technically be a problem doing it. However, merging
into another branch is not possible from GitHub's web interface as far
as I can see -- we would need to use the git
command line to merge
into a different branch, or re-do the pull request...
@riccardomurri This was tested and merged into develop.
This large commit merges the whole contents of repositories
TissueMAPS/JtLibrary
andTissueMAPS/JtModules
, preserving the history of each.Contents are then rearranged: source files are now split by programming language first (with top-level directories
matlab/
,python/
, andr/
) and by "package" second (with second-level dirsjtmodules/
andjtlib/
).This preserves the ability to install separate packages from the joint repo; for instance, you would need to run
pip install .
in both thepython/jtmodules/
and thepython/jtlibrary/
directories. (Some modifications to thesetup.py
files have been necessary.)I have kept the
handles/
directory at top-level, assuming that handles can refer to modules written in any language.