Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Feature/add atlas data import #174

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

[WIP] Feature/add atlas data import #174

wants to merge 14 commits into from

Conversation

a-solovyev12
Copy link
Member

[WIP] This PR adds wrappers for new scripts from atlas-data-import repo. It also merges a different version of expression data import script with the existing one, ensuring backward-compatibility.

@a-solovyev12 a-solovyev12 requested a review from pcm32 June 22, 2020 15:22
pcm32
pcm32 reviewed Jun 23, 2020
View reviewed changes
Copy link
Member

@pcm32 pcm32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work! I would suggest though that we unify most of this inside a single downloader with options. Otherwise we have an inflation of tools and users need to be installing different tools for particular files of Atlas that they are probably not aware of. Also, the classifier part we need to discuss how to release, in my view this is not releaseable as a toolshed tool until we don't have release data for this.

tools/tertiary-analysis/data-scxa/atlas_import_experiment_data.xml Outdated
@@ -0,0 +1,141 @@
<tool id="atlas_import_experiment_data" name="Atlas import: get experiment data" version="@TOOL_VERSION@+galaxy0" profile="@PROFILE@">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is replacing retrieve-scxa, I would suggest that it uses the same file name, tool id and possibly tool name than the previous version, with a good bump in version.

tools/tertiary-analysis/atlas-data-import/atlas_import_sdrf_files.xml
@@ -0,0 +1,33 @@
<tool id="atlas_import_sdrf_files" name="Atlas import: get sdrf files" version="@TOOL_VERSION@+galaxy0" profile="@PROFILE@">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is worth having a whole tool just for this, I would add it to the main downloader with an optional activation switch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcm32 I was thinking so too initially, but then decided to make a separate tool, because it's a different use case. In atlas-data-import, we specify single accession and import data. Here, the tool finds available classifiers for all datasets, unless specified otherwise, and imports them. Putting all this into one script will be a mess.

tools/tertiary-analysis/atlas-data-import/atlas_import_experiment_data.xml
@@ -0,0 +1,63 @@
<tool id="atlas_import_experiment_data" name="Atlas import: get experiment data" version="@TOOL_VERSION@+galaxy0" profile="@PROFILE@">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be all options of the main downloader I would say. Otherwise we clutter with an unnecessarily high number of tools.

tools/tertiary-analysis/atlas-data-import/atlas_import_experiment_data.xml
--config-file "${config_file}"
#end if
#if $decorated_rows
--decorated-rows "${decorated_rows}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this for instance is already part of the main downloader I think.

tools/tertiary-analysis/atlas-data-import/atlas_import_classifiers.xml
@@ -0,0 +1,29 @@
<tool id="atlas_import_classifiers" name="Atlas import: get classifiers" version="@TOOL_VERSION@+galaxy0" profile="@PROFILE@">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that we keep this in a separate branch. We cannot merge this and add it to the toolshed until classifier data is not a part of the Atlas SC release.

tools/tertiary-analysis/data-scxa/retrieve-sdrf-files.xml Outdated
@@ -0,0 +1,33 @@
<tool id="atlas_import_sdrf_files" name="Atlas import: get sdrf files" version="@TOOL_VERSION@+galaxy0" profile="@PROFILE@">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be part of the main downloader.

pcm32
pcm32 reviewed Jul 9, 2020
View reviewed changes
tools/tertiary-analysis/data-scxa/retrieve-scxa.xml
@@ -5,13 +5,22 @@
</macros>
<expand macro="requirements" />
<command detect_errors="exit_code"><![CDATA[
get_experiment_data.R --accesssion-code "${accession_code}" --matrix-type "${matrix_type}" --get-sdrf "${get_sdrf}" --get-condensed-sdrf "${get_condensed_sdrf}" --get-marker-genes "${get_marker_genes}"
ln -s "${accession_code}_${matrix_type}/10x_data/matrix.mtx" matrix.mtx &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases I tend to move directly into the output variable (and that spares you the extra from_work_dir), as the working directory will be deleted at the end of the job, but if this is working it means that Galaxy is grabbing it before the cleanup, so it should be fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be more natural to do soft links after running the command? at this point the files don't exist yet, it didn't complain because of that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcm32 Symlinks work fine with non-existent files, I've checked it. The reason why I put them before the main script is that we don't know in advance which if-block will be the last one, so we can't correctly place && in advance to combine two commands.

@a-solovyev12
fix conflict w develop
f6a8d0f
@a-solovyev12
update atlas-data-import to 0.0.11 (#189)
125cf46 
* add optional inputs to scmap-cell (#182)

* add optional inputs to scmap-cell

* increment version number

* update tool versions (#183)

* add default values to parameters (#184)

* add default values to parameters

* bump wrapper version

* update scmap-cli to 0.0.10 (#185)

* update scmap-cli to 0.0.10

* Switch to the conda version of planemo in .travis.yml

* Revert "Switch to the conda version of planemo in .travis.yml"

This reverts commit 1693b3e.

* pin planemo=0.70.0 and bioblend=0.13.0

* pin planemo=0.62.1

* pin planemo=0.70.0 and bioblend=0.14.0

* pin planemo=0.70.0 and bioblend=0.12.0

* specify python3

* check python version

* add pip upgrade; remove package versions

* add pip upgrade

* specify python3 when installing planemo

* Revert "specify python3 when installing planemo"

This reverts commit c02ec7b.

* specify pip3 version of packages

* specify pip3 version of packages

* Revert "specify pip3 version of packages"

This reverts commit 28d7f35.

* Revert "specify pip3 version of packages"

This reverts commit 215630c.

* Revert "Revert "specify python3 when installing planemo""

This reverts commit 0cdf27a.

* Revert "specify python3 when installing planemo"

This reverts commit c02ec7b.

* Revert "add pip upgrade"

This reverts commit d75c05a.

* Revert "add pip upgrade; remove package versions"

This reverts commit f595c7d.

* Revert "check python version"

This reverts commit d095f1e.

* Revert "specify python3"

This reverts commit 7c6de29.

* Revert "pin planemo=0.70.0 and bioblend=0.12.0"

This reverts commit 784945d.

* Revert "pin planemo=0.70.0 and bioblend=0.14.0"

This reverts commit e4e8fe0.

* Revert "pin planemo=0.62.1"

This reverts commit 81da516.

* Revert "pin planemo=0.70.0 and bioblend=0.13.0"

This reverts commit 7c78b39.

* Revert "Revert "Switch to the conda version of planemo in .travis.yml""

This reverts commit 8312aaa.

* Revert "Switch to the conda version of planemo in .travis.yml"

This reverts commit 1693b3e.

* Specify gxformat2=0.11.1 when installing planemo

* specify package version using ==

* make sure pip3 is installed

* pin planemo==0.70

* update scmap and scpred wrappers to provide correct output format (#186)

* update scmap and scpred wrappers to provide correct output format

* switch to 'tabular'

* bump tool version

* Feature/update scmap and scpred (#187)

* update scmap and scpred wrappers to provide correct output format

* switch to 'tabular'

* bump tool version

* bump the tool version (for real)

* update atlas-data-import to 0.0.11

* Revert "Merge branch 'develop' of https://github.com/ebi-gene-expression-group/container-galaxy-sc-tertiary into feature/add_import_retries"

This reverts commit b911912, reversing
changes made to 251045e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcm32 pcm32 pcm32 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@a-solovyev12 @pcm32