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

isIncluded for all data types? #29

Closed
PavelBal opened this issue Feb 1, 2022 · 12 comments · Fixed by #44 or #45
Closed

isIncluded for all data types? #29

PavelBal opened this issue Feb 1, 2022 · 12 comments · Fixed by #44 or #45
Assignees

Comments

@PavelBal
Copy link
Member

PavelBal commented Feb 1, 2022

Following from the documentation of the function isIncluded it is not clear to me, which data types are supported. Probably only atomic, and it cannot check whether e.g. a certain DataSet is inside a list of DataSet? In this case, it should be documented accordingly.

@PavelBal
Copy link
Member Author

PavelBal commented Feb 1, 2022

> isIncluded(c(1,2,3), list(c(1,2,3)))
[1] FALSE

So definitely only for atomics...

@PavelBal
Copy link
Member Author

PavelBal commented Feb 1, 2022

> isIncluded(1, "1")
[1] TRUE

(veryconfused) ok lol, I don't like this function :D

@IndrajeetPatil
Copy link
Member

This behavior actually has nothing to do with this function and everything to do with implicit and arcane coercion rules of R

1 %in% "1"
#> [1] TRUE

Created on 2022-02-02 by the reprex package (v2.0.1.9000)

@msevestre
Copy link
Member

Lol. Arcane coercion rules..

I am going to name this the truth of the month. Respect @IndrajeetPatil

IndrajeetPatil added a commit that referenced this issue Feb 2, 2022
- better documentation for `isIncluded()`
- more tests
- more examples
@IndrajeetPatil IndrajeetPatil linked a pull request Feb 2, 2022 that will close this issue
@IndrajeetPatil
Copy link
Member

@PavelBal Is this what you want to do?

it cannot check whether e.g. a certain DataSet is inside a list of DataSet

> library(ospsuite)
> library(ospsuite.utils)
> 
> # import observed data
> dataSet <- loadDataSetsFromExcel(
+   xlsFilePath = file.path(getwd(), "tests", "data", "CompiledDataSetStevens2012.xlsx"),
+   importerConfiguration = DataImporterConfiguration$new(file.path(getwd(), "tests", "data", "ImporterConfiguration.xml"))
+ )
> 
> isIncluded("Stevens_2012_placebo.Placebo_total", names(dataSet))
[1] TRUE

@PavelBal
Copy link
Member Author

PavelBal commented Feb 2, 2022

Is this what you want to do?

No, I meant that it must be documented that it only works on atomic data types. E.g. this would probably not work:

ds1 <- DataSet$new("DataSet1")
ds2 <- DataSet$new("DataSet1")
ds3 <- DataSet$new("DataSet1")

isIncluded(ds1, list(ds1, ds2, ds3))

And as already shown before - it does not work on lists:

> isIncluded(c(1,2,3), list(c(1,2,3)))
[1] FALSE

@IndrajeetPatil
Copy link
Member

IndrajeetPatil commented Feb 2, 2022

it only works on atomic data types

The function works with a vector of values, be it an atomic or non-atomic vector. So all you need to do for this to work with DataSet objects, is to convert the argument to a vector.

library(ospsuite)
#> Loading required package: rClr
#> Loading the dynamic library for Microsoft .NET runtime...
#> Loaded Common Language Runtime version 4.0.30319.42000
library(ospsuite.utils)

ds1 <- DataSet$new(name = "DataSet1")
ds2 <- DataSet$new(name = "DataSet1")
ds3 <- DataSet$new(name = "DataSet1")

is.vector(ds1)
#> [1] FALSE
is.vector(c(ds1))
#> [1] TRUE

isIncluded(ds1, list(ds1, ds2, ds3))
#> Error in match(x, table, nomatch = 0L): 'match' requires vector arguments
isIncluded(c(ds1), list(ds1, ds2, ds3))
#> [1] TRUE

Created on 2022-02-02 by the reprex package (v2.0.1.9000)

it does not work on lists

It works with lists, and it's telling you correctly that the first vector is not contained in the second list. In fact, they are not even of the same length: vector has a length of 3, while list has a length of 1. So the function is working as expected.

library(ospsuite.utils)

length(c(1, 2, 3))
#> [1] 3
length(list(c(1, 2, 3)))
#> [1] 1

isIncluded(c(1, 2, 3), list(c(1, 2, 3)))
#> [1] FALSE

isIncluded(c(1, 2, 3), list(1, 2, 3))
#> [1] TRUE
isIncluded(list(1, 2, 3), c(1, 2, 3))
#> [1] TRUE

Created on 2022-02-02 by the reprex package (v2.0.1.9000)

msevestre pushed a commit that referenced this issue Feb 2, 2022
- better documentation for `isIncluded()`
- more tests
- more examples
@PavelBal
Copy link
Member Author

PavelBal commented Feb 2, 2022

isIncluded(ds1, list(ds1, ds2, ds3))
#> Error in match(x, table, nomatch = 0L): 'match' requires vector arguments
isIncluded(c(ds1), list(ds1, ds2, ds3))
#> [1] TRUE

Sorry, but this just does not work. You cannot use e.g. R6 objects with this function.

> isIncluded(c(ds1), list(ds2))
[1] TRUE

> isIncluded(c(ds1), list(DataSet))
[1] TRUE

I am ok with this as long it is clear to the user ( == documented properly).

@PavelBal PavelBal reopened this Feb 3, 2022
IndrajeetPatil added a commit that referenced this issue Feb 3, 2022
@IndrajeetPatil IndrajeetPatil linked a pull request Feb 3, 2022 that will close this issue
msevestre pushed a commit that referenced this issue Feb 3, 2022
* closes #29

* if not a vector, convert to one
@PavelBal PavelBal reopened this Feb 4, 2022
@PavelBal
Copy link
Member Author

PavelBal commented Feb 4, 2022

Re-opening it because the behavior is currently erroneous regarding non-primitive types

So this function only works for primitive types basically.
Does it make sense to say that we throw an error if it is used with something else than char, int, double, boolean etc..?

Yes I would vote for this. This function has (hopefully) not been used with R6 classes or whatever, so let's not invest time unless we have a need for it.

BTW yes, identical is cool, I think we all should read the documentation for it and consider it in our code (especially in if statements).

> NA == NA
[1] NA
> identical(NA, NA)
[1] TRUE

Hell yeah, THIS makes sense!

@PavelBal
Copy link
Member Author

PavelBal commented Feb 4, 2022

@IndrajeetPatil
Copy link
Member

Haha

There is an entire book on the weird behaviour R sometimes exhibit:

image

@IndrajeetPatil
Copy link
Member

To be fair to R though, it's not unique in this regard. Watch the notorious WAT talk for a similar take on JavaScript:

https://www.youtube.com/watch?v=et8xNAc2ic8

IndrajeetPatil added a commit that referenced this issue Feb 4, 2022
error if objects are entered as arguments
IndrajeetPatil added a commit that referenced this issue Feb 10, 2022
* Create develop branch

* closes #29 (#44)

- better documentation for `isIncluded()`
- more tests
- more examples

* closes #29 (#45)

* closes #29

* if not a vector, convert to one

* closes #29 (#46)

* closes #29

error if objects are entered as arguments

* drop purrr

* don't accept environments either (#47)

just for good measure

* Fixes #32 set a more consistent behaviour for formatNumerics (#48)

* closes #30 (#49)

* closes #31 (#52)

* closes #50 (#51)

remove unnecessary lint detection

* closes #34 (#54)

Separates docs for `validateIsSameLength()` from `validateIsOfLength()`

* closes #21; prepare for CRAN (#55)

* closes #21; prepare for CRAN

* more tests with list

* use correct URL for codecov in README (#56)

Co-authored-by: Michael Sevestre <[email protected]>
Co-authored-by: Pierre Chelle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants