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

make order features applied in consistent #1177

Merged
merged 1 commit into from
Mar 25, 2023

Conversation

hcoles
Copy link
Owner

@hcoles hcoles commented Mar 25, 2023

Fixes #1175

@hcoles hcoles force-pushed the bug/consistent_feature_order branch from 63854cb to 1ceafb3 Compare March 25, 2023 13:32
@davidburstrom
Copy link
Contributor

Is it an option to use https://stackoverflow.com/a/51945053/643007 ?

@hcoles
Copy link
Owner Author

hcoles commented Mar 25, 2023

That would keep the order consistent, but not make it (easily) predictable.

@davidburstrom
Copy link
Contributor

True. I'm just thinking that it would preserve the order of the input (e.g. the list of features), whereas if the alphabetical output order isn't viable, it needs to be shuffled on the receiving end. It all depends on the intended contract for FeatureSelector.

@hcoles
Copy link
Owner Author

hcoles commented Mar 25, 2023

Order wasn't meant to be important, and hence hadn't been considered before now. Without the explicit alphabetical order, it would be determined by the order plugins are picked up off the classpath. That would introduces further unpredictability and the possibility of different behaviour for the same codebase when run on a different os/some other factor changes. It's easier going forward to have a simple explicit order that a human can understand.

@davidburstrom
Copy link
Contributor

Got it. It's true that the crash in the interceptors was only coincidentally related to ordering, it was just a consequence of the argument swapper interceptor being buggy. Let's just hope that the new order doesn't cause any other intermittent issues.

@hcoles hcoles merged commit 081bddf into master Mar 25, 2023
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.

Regression with Pitest 1.11.6
2 participants