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.

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

[24.1] Fill in missing help for cross product tools. #18698

Merged

Conversation

jmchilton
Copy link
Member

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@jmchilton
Copy link
Member Author

jmchilton commented Aug 14, 2024

Update: Deleted dated screenshots.

@jmchilton jmchilton force-pushed the 241_crossproduct_help branch 2 times, most recently from 0c3e51e to 2873f70 Compare August 14, 2024 22:44
@jmchilton jmchilton changed the title [WIP][24.1] Fill in missing cross product help... [24.1] Fill in missing help for cross product tools. Aug 14, 2024
@jmchilton jmchilton marked this pull request as ready for review August 14, 2024 22:46
@github-actions github-actions bot added this to the 24.1 milestone Aug 14, 2024
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Thanks for the efforts.

What this tool does (technical details)
============================================

This tool consumes two lists - we will call them ``input_a`` and ``input_b``. If ``input_a``
Copy link
Contributor

Choose a reason for hiding this comment

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

Would call those input A and B. And the lists a_1,...a_n and b_1,...,b_m.

Copy link
Member

Choose a reason for hiding this comment

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

@bernt-matthias The text doesn't refer to the inputs at all, I don't think we need to name them.

I would perhaps just say

Suggested change
This tool consumes two lists - we will call them ``input_a`` and ``input_b``. If ``input_a``
This tool consumes two flat lists. We will call the input collections ``input_a`` and ``input_b``. If ``input_a``

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, why is it called input_a?

My main point would be not to use indices (with underscores) here (and instead use it for the list elements).

Copy link
Member

Choose a reason for hiding this comment

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

because it's input collection a and input collection b ? I don't get the point about underscores.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's my expectation from the more formal/mathematical papers I encountered. With underscores I expect elements of a list.

That is, my preference would be underscores for the lists and no underscores for the two inputs.

In the end I don't care that much. Formally it's well defined and fine. Just a bit more pleasant for me 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a math background and not having the underscores physically pains me to look at 😆 - I think I did it anyway because I wanted them to seem like Galaxy dataset an actual bench scientist would have and not like mathematical concepts.

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Awesome, and I'm sorry that I said I would write the text and then didn't 😅

lib/galaxy/tools/cross_product_flat.xml Outdated Show resolved Hide resolved
lib/galaxy/tools/cross_product_nested.xml Outdated Show resolved Hide resolved
lib/galaxy/tools/cross_product_nested.xml Outdated Show resolved Hide resolved
Comment on lines +33 to +34
This matching up of elements is a very natural way to "map" an operation (or in Galaxy parlance, a tool)
over two lists. However, sometimes the desire is to compare each element of the first list to each element of the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This matching up of elements is a very natural way to "map" an operation (or in Galaxy parlance, a tool)
over two lists. However, sometimes the desire is to compare each element of the first list to each element of the
This matching up of elements is a very natural way to perform a "map over" operation, which in Galaxy means to map an operation over the elements of multiple lists. However, sometimes the desire is to compare each element of the first list to each element of the

I think I would want to establish the term "map over" (and quote it) since that's what (I think) we use when we're talking about these operations. It has probably entered the conversation in chats and the help forum at this point.

Feel free to dismiss this though, it's great that we finally put it into writing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have opened a PR that I think would enable us to do this well in dev #18722.

What this tool does (technical details)
============================================

This tool consumes two lists - we will call them ``input_a`` and ``input_b``. If ``input_a``
Copy link
Member

Choose a reason for hiding this comment

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

@bernt-matthias The text doesn't refer to the inputs at all, I don't think we need to name them.

I would perhaps just say

Suggested change
This tool consumes two lists - we will call them ``input_a`` and ``input_b``. If ``input_a``
This tool consumes two flat lists. We will call the input collections ``input_a`` and ``input_b``. If ``input_a``

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Please go ahead. I'm happy with this excellent contribution.

@jmchilton jmchilton merged commit 33d3fa4 into galaxyproject:release_24.1 Aug 20, 2024
43 of 47 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

@nsoranzo nsoranzo deleted the 241_crossproduct_help branch August 20, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants