Update table compute - Change rangemaker logic to allow pandas like indexing

[paulzierep]

FOR CONTRIBUTOR:

• I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
• License permits unrestricted use (educational + commercial)
• This PR adds a new tool or tool collection
• This PR updates an existing tool or tool collection
• This PR does something else (explain below)

Fix #5783
This was annoying me for a long time, maybe it can be fixed unit I am back in September ;)

[wm75]

I like this! Before I start reviewing it more carefully: is there any functionality that you'll loose with this change? Doesn't look like from a first glance, but just to make sure :-)

Other question: when we're moving that close to actual pandas syntax, is it worthwhile keeping the 1-based positive indexing, or would it be better to switch to pandas-style completely?

[paulzierep]

I like this! Before I start reviewing it more carefully: is there any functionality that you'll loose with this change? Doesn't look like from a first glance, but just to make sure :-)

Other question: when we're moving that close to actual pandas syntax, is it worthwhile keeping the 1-based positive indexing, or would it be better to switch to pandas-style completely?

I thought the same, usually we use 1-based indexing for most column-related tools in Galaxy, that was the reason, otherwise we can keep pandas / python based indexing. We could make it optionally, but that might confuse more than help. I really just want to get rid of the current indexing nonsense.

[wm75]

tools-iuc/tools/table_compute/table_compute.xml

Line 96 in 9e338cf

<requirement type="package" version="1.2.4">pandas</requirement>

Looks like this should reuse @VERSION@

[wm75]

isn't that the old behavior instead of the one you're aiming for?

[wm75]

same here?

[wm75]

I think the change also deserves highlighting somewhere. Otherwise this can be a very unpleasant surprise for people updating, say, a workflow to use the new tool version and suddenly performing calculations on the wrong columns.
Probably a warning box at the very top of the tool help?

@bgruening do you think that would be enough? or a banner? but then that's only visible on .eu, or is it a bad idea after all to have such behavior change in the tool?

[bgruening]

I had hoped to stay out of this :)

What about changing the param name as well ... this way people are forced to look more closely at the param. We can then also add a warning to the param and the help section as well. Which we remove in a few versions.

[wm75]

That's a good idea! It will cause many false alerts for people who aren't using slices with negative indices, but better this way than introducing silent bugs I think.

[paulzierep]

I had hoped to stay out of this :)

What about changing the param name as well ... this way people are forced to look more closely at the param. We can then also add a warning to the param and the help section as well. Which we remove in a few versions.

You mean Name or Label ? So changing what the user sees, or actually changing the name ?

[paulzierep]

Will make an update with the correct logic today, still up to discussion is if we should use 0-based indexing, then people can fully rely on pandas based indexing ? Maybe this can be part of the hackathon ? I do not mind either way as long as we inform the user.

[wm75]

changing the actual parameter name so that tool updates inside a workflow will break connections.

[paulzierep]

changing the actual parameter name so that tool updates inside a workflow will break connections.

ok now I got the logic

[wm75]

wait, another idea, maybe silly:

what if you'd disallow slices with negative stops in the current version, but introduce alternative rust-like syntax for slices using .. as the start..stop separator, and in that new alternative syntax, and only there, allow negative stops?

That way users that don't use expressions affected by the change could update to the new version without hassle, but if you are using them you have to conciously change to the new syntax.

[wm75]

and one more note: your changes introduce a new inconsistency.
While 1:-2 now behaves like pandas df.iloc, -2:2 still wraps around in forward direction creating a new asymmetry.
If you want to be consistent with iloc, you need to disallow cases where left < 0, but right >=0.

[wm75]

wait, another idea, maybe silly:

what if you'd disallow slices with negative stops in the current version, but introduce alternative rust-like syntax for slices using .. as the start..stop separator, and in that new alternative syntax, and only there, allow negative stops?

That way users that don't use expressions affected by the change could update to the new version without hassle, but if you are using them you have to conciously change to the new syntax.

Thinking about this more, a good way forward might be:

1. leave the current : slice syntax as it is entirely and mention it as legacy in the parameter help
2. introduce new, alternative .. syntax and implement the new behavior just for this one

This way, no exisiting usage breaks, and there are no surprises either. The new implementation can then also use 0-based indexing.

[paulzierep]

wait, another idea, maybe silly:
what if you'd disallow slices with negative stops in the current version, but introduce alternative rust-like syntax for slices using .. as the start..stop separator, and in that new alternative syntax, and only there, allow negative stops?
That way users that don't use expressions affected by the change could update to the new version without hassle, but if you are using them you have to conciously change to the new syntax.

Thinking about this more, a good way forward might be:

1. leave the current : slice syntax as it is entirely and mention it as legacy in the parameter help
2. introduce new, alternative .. syntax and implement the new behavior just for this one

This way, no exisiting usage breaks, and there are no surprises either. The new implementation can then also use 0-based indexing.

I agree, the best solution without breaking stuff but also allowing new logic would be the introduction of a new parameter with pandas based slicing. Since I am out of time before parental leave, I would propose this as a nice project for the tools' hackathon. In my opinion, I would completely revert my changes to rangemaker and just use the 0-based pandas iloc logic directly as the other parameter. This way users can apply pandas slicing, for which there are plenty of docs.