Skip to content

Conversation

@LeilyR
Copy link
Contributor

@LeilyR LeilyR commented Jul 23, 2020

Welcome to deepTools GitHub repository! Please check the following regarding
your pull request :

  • Does the PR contain new feature?
  • Does the PR contain bugfix?
  • Does the PR contain documentation changes?
  • Does the PR contain changes to the galaxy wrapper?

Added sortUsingSamples and clusterUsingSamples to galaxy wrapper as has been asked in #976

@LeilyR LeilyR requested review from bgruening and dpryan79 July 23, 2020 12:53
Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Small review added.

<when value="yes">
<expand macro="sortRegions" />
<expand macro="sortUsing" />
<param argument="--sortUsingSamples" type="text" size="30"
Copy link
Member

Choose a reason for hiding this comment

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

size can be removed it is not used anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then I remove it from other options too

<param argument="--sortUsingSamples" type="text" size="30"
label="List of samples to be used for sorting"
help="List of sample numbers (order as in matrix), which are used by --sortUsing for sorting.
If no value is set, it uses all samples. Example: 1 3 (space separated!)">
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add:

            <sanitizer invalid_char="">
                <valid initial="string.digits">
                    <add value=" "/>
                </valid>
            </sanitizer>

This will make sure only digits and space and be entered.

help="The default is to make one plot per bigWig file, i.e., all samples next to each other. Choosing this option will make one plot per group of regions."/>

<expand macro="kmeans_clustering" />
<param argument="--clusterUsingSamples" type="text" size="30"
Copy link
Member

Choose a reason for hiding this comment

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

size can be removed

<param argument="--clusterUsingSamples" type="text" size="30"
label="List of samples to be used for clustering"
help="List of sample numbers (order as in matrix), which are used by --kmeans or --hclust for clustering.
If no value is set, it uses all samples. Example: 1 3 (space separated!)">
Copy link
Member

Choose a reason for hiding this comment

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

sanitizer should be added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgruening I have already changed them. Is it good now? If so, I am gonna merge it.

@bgruening bgruening merged commit f773fdf into develop Aug 21, 2020
@bgruening
Copy link
Member

Yes, that looks great. Thanks @LeilyR!

@bgruening bgruening deleted the fix_976 branch August 21, 2020 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants