Open-Systems-Pharmacology / OSPSuite.RUtils

Utility functions for Open Systems Pharmacology R packages
https://www.open-systems-pharmacology.org/OSPSuite.RUtils/dev/
GNU General Public License v2.0
1 stars 3 forks source link

isEmpty() does not behave correctly with the empty string #121

Open Yuri05 opened 1 year ago

Yuri05 commented 1 year ago
> ospsuite.utils::isEmpty("")
[1] FALSE
PavelBal commented 1 year ago

It's not a bug, it's a feature :)

The function tests if "An object or an atomic vector or a list of objects" is empty. "" is a vector of length 1 with one entry - the empty string.

> length("sd")
[1] 1
> length("")
[1] 1

Furthermore, "" is a valid unit string for the dimension Dimensionless, so its not empty...

Yuri05 commented 1 year ago

well, in the most languages isempty(<empty string>) returns true. e.g. in Matlab, isempty returns true for both empty array and empty string:

But if everybody is fine with the current implementation - I can live with it.

msevestre commented 1 year ago

I would assume it's a bug. An empty string is always empty In which scenario does someone checks is empty ("") and does something meaningful when it's true? Can you explain @PavelBal ?

msevestre commented 1 year ago

And isEmpty for the "" unit should reurnr true so that we don't put [] for nothing

PavelBal commented 1 year ago

isEmpty() checks if the object is empty. In R's world, "" is a vector of type character with one element - an empty string. We can even access this first element:

> ""[[1]]
[1] ""

An empty objects/structure would be character():

> isEmpty(character())
[1] TRUE

And we indeed cannot access the first element:

> c <- character()
> c[[1]]
Error in c[[1]] : subscript out of bounds

And while I agree that this may be not intuitive, the behavior of this function is correct from R's perspective and I would not change it. You can read isEmpty() in the way "are there elements that I can subset?", and the answer for "" is "Yes".

msevestre commented 1 year ago

what about we create a method areThereElementsToSubset ?

PavelBal commented 1 year ago

Or isEmptyString()? :) In this case we would not change the current implementation and potentially break some dependencies.

IndrajeetPatil commented 1 year ago

I guess what you have in mind is nchar():

nchar("")
#> [1] 0

Created on 2022-12-11 with reprex v2.0.2

Btw, if you plan to create a new function isEmptyString(), you can repurpose the existing logic from (vectorized) hasEmptyStrings():

https://github.com/Open-Systems-Pharmacology/OSPSuite.RUtils/blob/409d2382eb49f04e9e332a126452b89b0cc2a6e5/R/validation-emptiness.R#L93-L95