csgillespie / benchmarkme

Crowd sourced benchmarking
https://csgillespie.github.io/benchmarkme/
40 stars 13 forks source link

added logic to handle Windows 10/11 ram info in vector form #45

Closed xiaodaigh closed 2 years ago

xiaodaigh commented 2 years ago

fixes #44

csgillespie commented 2 years ago

@ArkaB-DS If you have time, could you try this fix out?

I suspect not all Windows machines are the same(!) which is why we are running into issues

ArkaB-DS commented 2 years ago

@ArkaB-DS If you have time, could you try this fix out?

I suspect not all Windows machines are the same(!) which is why we are running into issues

This works for me. However, note that in my case

get_windows_ram()
[1] "8589934592  \r" "\r" 

So, I get

> ram = get_windows_ram()
> sum(sapply(ram, clean_win_ram))
[1] NA

I think the "na.rm = T" argument should be added to "sum". Further, @xiaodaigh , instead of return(sum(sapply(ram, clean_win_ram))) can you try return(clean_win_ram(ram)) after changing the clean_win_ram function to -

clean_win_ram = function(ram) {
  sum(as.numeric(ram), na.rm = T)
}
csgillespie commented 2 years ago

Thanks @ArkaB-DS !

@xiaodaigh Are you OK to update your PR

xiaodaigh commented 2 years ago

ram = ram[nchar(ram) > 0L] doesn't ths line prevent issues?

Using this branch? Can u call get_ram() directly @ArkaB-DS ?

image

ArkaB-DS commented 2 years ago

ram = ram[nchar(ram) > 0L] doesn't ths line prevent issues?

Using this branch? Can u call get_ram() directly @ArkaB-DS ?

image @xiaodaigh


clean_ram = function(ram, os) {
ram = stringr::str_squish(ram)
ram = ram[nchar(ram) > 0L]
# Some Windows machine with multiple physical RAM modules will report RAM in a
# vector hence this logic to handle that case
if(.Platform$OS.type == "windows" && length(ram) > 1) {
return(sum(sapply(ram, clean_win_ram)))
}

if (length(ram) > 1 ||

is.na(ram) ||

length(grep("^solaris", os))) { # Don't care about solaris

return(NA)

}

#

if (length(grep("^linux", os))) {

clean_ram = clean_linux_ram(ram)

} else if (length(grep("^darwin", os))) {

clean_ram = clean_darwin_ram(ram) # nocov

} else {

clean_ram = clean_win_ram(ram) # nocov

}

unname(clean_ram)

}

On cleaning and rebuilding with the above, I get - 

library(benchmarkme) get_ram() Error in abs(x) : non-numeric argument to mathematical function In addition: Warning message: In structure(cleaned_ram, class = "ram") : Calling 'structure(NULL, )' is deprecated, as NULL cannot have attributes. Consider 'structure(list(), )' instead.


This occurs as my ram = "8589934592" - a single character vector (after `ram = ram[nchar(ram) > 0L]`).
So, `length(ram)>1` fails.

And yes, you are correct, ram = ram[nchar(ram) > 0L] removes the NA-causing part - I missed that.

xiaodaigh commented 2 years ago

yeah so since after the change, you only a single character vector then you've commented out the rest

8589934592 suspect this number is just too big

also, why did comment out the below code? which has a logic to handle your situation I think.

ArkaB-DS commented 2 years ago

ram = ram[nchar(ram) > 0L] doesn't ths line prevent issues?

Using this branch? Can u call get_ram() directly @ArkaB-DS ?

image

@xiaodaigh I was checking if only this part suffices for my case which I thought was your question. The entire code, along with your suggestion, works fine for me now.