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

feat: add check for permutation #230

Merged
merged 6 commits into from
Apr 25, 2023
Merged

feat: add check for permutation #230

merged 6 commits into from
Apr 25, 2023

Conversation

sebffischer
Copy link
Contributor

No description provided.

@sebffischer
Copy link
Contributor Author

The only thing I am unhappy with is the any(x != y), there should be a faster way to test this. Any suggestions? But I could not quickly find a function that does that. At least identical might be tricky with factors. Maybe some version of all.equal?

@mllg
Copy link
Owner

mllg commented Aug 1, 2022

A more general question: What is the intended behaviour if x or y have duplicated values? I'm a bit confused by the comment

   # now this is checkSetEqual(..., ordered = TRUE) with additional sorting.

@mllg
Copy link
Owner

mllg commented Aug 1, 2022

Also, x != y will not work with missing values.

Edit: Well, at least not in all cases, e.g. any(c(FALSE,NA) != c(FALSE, NA))

@sebffischer
Copy link
Contributor Author

Duplicated values are not treated special in any way or what exactly do you mean by that?

I think the comment regarding the checkSetEqual with ordered = TRUE is actually incorrect and should be removed. What I mean by that is that comparing [1, 2, 1] with [1, 1, 2] essentially turns into checking whether all [1, 1, 2] == [1, 1, 2]. This also shows how the duplicated values are being handled.

Yes, but the x != y never contains missing values (they are dropped by the sort, see the comment).

@mllg
Copy link
Owner

mllg commented Aug 1, 2022

Sorting comes after the length check, so this fails?

x = c(1, NA)
y = c(1)

@sebffischer
Copy link
Contributor Author

sebffischer commented Aug 1, 2022

Yes. Note that we check the length twice.

  1. Before sorting: To check the lengths of the full vectors.
  2. After sorting: To check whether the same amount of NAs was removed

Your example is captured in the first length check

Edit: I mean "Yes, this returns FALSE"

@mllg
Copy link
Owner

mllg commented Aug 1, 2022

Ok, got it now. I think you could clarify a bit in the documentation, but otherwise good to be merged.

@sebffischer
Copy link
Contributor Author

I added some comments

@sebffischer
Copy link
Contributor Author

@mllg

@sebffischer
Copy link
Contributor Author

Can this be merged @mllg ?

@mllg mllg merged commit 7298271 into mllg:master Apr 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.

2 participants