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

Enforce consistent channel ordering in Pixie #1152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

Users may not always specify channels in strictly alphabetical order when running pixel-level Pixie. Additionally, Python's definition of sorting differs significantly from how we normally think of sorting, especially with capitalization. To prevent confusion, we need a consistent, non-stochastic ordering of channel names.

How did you implement your changes

Add a line to call Python's sorted function after defining the channels list. This ensures channels adheres to Python's sorting conventions.

Remaining issues

Not sure if cross-platform compatibility will be an issue, but sorted should be OS-agnostic.

@alex-l-kong alex-l-kong self-assigned this Aug 29, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cliu72
Copy link
Contributor

cliu72 commented Aug 30, 2024

@alex-l-kong I thought this was already addressed in this PR: #1114 (See point that says "Adds natsort calls throughout the Pixie pipeline to avoid issues with channel ordering." under "Pixel clustering"). Does this PR address something different?

@alex-l-kong
Copy link
Contributor Author

@alex-l-kong I thought this was already addressed in this PR: #1114 (See point that says "Adds natsort calls throughout the Pixie pipeline to avoid issues with channel ordering." under "Pixel clustering"). Does this PR address something different?

Let me check with Avery again. It looked like he had updated the repo but he was still running into this issue, so I added this as an additional safeguard.

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.

2 participants