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 sure interpreted and compiled orderings agree #4871

Merged
merged 5 commits into from Dec 4, 2018

Conversation

@cseed
Copy link
Collaborator

@cseed cseed commented Dec 3, 2018

Builds on: #4869

ExtendedOrdering on rows and containers now matches CodeOrdering by using lt instead of compare in lt, etc.

FYI @tpoterba since this could potentially (e.g. comparing arrays with nans) cause a user-visible change in behavior.

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Dec 3, 2018

I'm fine with this.

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Dec 3, 2018

do you have an example of something that changed?

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Dec 3, 2018

(would like to include that in patch notes)

@cseed
Copy link
Collaborator Author

@cseed cseed commented Dec 3, 2018

Consider [NaN] < [0.0]. Before, array compare used compare (the total ordering) to compare elements, and would have returned true or false. Now, it always returns false (since NaN compare false to everything), and something similar for -0.0, and rows, sets and maps. I think that's it.

@cseed
Copy link
Collaborator Author

@cseed cseed commented Dec 3, 2018

But only when ExtendedOrdering is used, which I think is pretty rare, now, just in Interpret (so eval, top-level expressions) and aggregators.

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Dec 3, 2018

Is this technically a file format change? If I have something keyed by array<float64>, we've just invalidated the ordering.

@catoverdrive
Copy link
Contributor

@catoverdrive catoverdrive commented Dec 3, 2018

Ordering should always use compare, I think

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Dec 3, 2018

er, the interpreter isn't used for file formats. carry on

@cseed
Copy link
Collaborator Author

@cseed cseed commented Dec 3, 2018

Yes, what @catoverdrive said, so the file formats should be OK.

@catoverdrive
Copy link
Contributor

@catoverdrive catoverdrive commented Dec 4, 2018

rebase?

ext(pord).min(this.left, other.left).asInstanceOf[IntervalEndpoint],
ext(pord).max(this.right, other.right).asInstanceOf[IntervalEndpoint])
// min(this.left, other.left)
if (ext(pord).lt(this.left, other.left))
Copy link
Contributor

@catoverdrive catoverdrive Dec 4, 2018

I feel like these should maybe be using compare, no? otherwise (I think) we can get things like

hull([-0.0, 1.0], [0.0, 1.0]) = [0.0, 1.0]
hull([0.0, 1.0], [-0.0, 1.0]) = [-0.0, 1.0]

Copy link
Collaborator Author

@cseed cseed Dec 4, 2018

Yeah. In fact, I think Interval should use compare (total ordering) everywhere. That matches the old behavior, too. Fixed.

@danking danking merged commit a3e3652 into hail-is:master Dec 4, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants