Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

Adding IsDecreasing and IsIncreasing #61

Closed
wants to merge 1 commit into from

Conversation

zeroviscosity
Copy link

No description provided.

@btracey
Copy link
Member

btracey commented Nov 18, 2016

sort.Float64sAreSorted?

@jonlawlor
Copy link
Contributor

This would allow testing for strict monotonicity, for what it's worth. This approach would also be faster than sort.IsSorted. This kind of thing doesn't really come up for me though so I don't know how useful it would be.

@zeroviscosity
Copy link
Author

zeroviscosity commented Nov 18, 2016

The need to check for strictly increasing data came up for me this morning when working with some time series stuff. I'd added a basic version in the project I was working on and then thought I'd toss a more generic one into this repo in case it would be useful.

@btracey
Copy link
Member

btracey commented Nov 18, 2016

I've never needed a test for strict monotonicity, though of course that doesn't mean it's not useful. I'm not just not sure we want to be duplicating standard library functionality. It would be a bit slower, but would it be more useful to add a Unique([]float64) float64 function?

if !sort.Float64sAreSorted(s) || !floats.Unique(s) {
   // whatever
}

At the very least, we need to be careful with the English in the description because of NaN (or add more text).

@jonlawlor
Copy link
Contributor

I would expect it to be floats.IsUnique(...) and that floats.Unique would return the unique members. Speaking of which we could use a unique function like matlab's that also tells you the indexes of unique values both in the original slice and as a method of recreating the original slice from the unique slice.

@zeroviscosity
Copy link
Author

Good points about duplicating functionality and the NaN issue. I originally had IsStrictlyIncreasing and IsStrictlyDecreasing before making the strictness optional. In any event, that functionality may not be needed often enough by others to justify its inclusion.

@kortschak
Copy link
Member

floats.AreUnique?

What is the decision here?

@zeroviscosity
Copy link
Author

The floats README states that the functions avoid allocations, so that rules out most efficient uniqueness checks, I think. Without allocations, the only option would be the brute force approach of comparing every element with every other element, would it not? If it comes down to an O(n2) approach without allocations or a faster approach that requires overhead, is it worth including in this package?

@btracey
Copy link
Member

btracey commented Mar 25, 2017

floats.AreUnique seems like a useful function, and it treats the slice as an entire unit. The map allocation is fine.

@btracey
Copy link
Member

btracey commented Mar 25, 2017

I'm still not sold on IsIncreasing, etc. though

@zeroviscosity
Copy link
Author

I'm happy to scrap IsIncreasing and IsDecreasing. If you're OK with a map allocation for AreUnique, would you rather I rework/rename this PR or close it and start a new one?

@btracey
Copy link
Member

btracey commented Mar 31, 2017

I think a new PR would be better, although we're in the process of trying to merge all the repos together, so if it's not super time critical, maybe it can be one of the first PRs in the new combined repo?

@zeroviscosity
Copy link
Author

Makes sense. I'll close this one out then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants