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

Rewrite of how to count threads used by current future plan. #421

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

Lenostatos
Copy link
Contributor

New commit directly to master for PR #418.

@Jean-Romain Jean-Romain self-assigned this Apr 8, 2021
@Jean-Romain Jean-Romain added Bug A bug in the package Enhancement Not actually a bug but a possible improvement labels Apr 8, 2021
@Jean-Romain Jean-Romain merged commit befbec5 into r-lidar:master Apr 8, 2021
@Jean-Romain
Copy link
Collaborator

Thanks

@Lenostatos
Copy link
Contributor Author

You're welcome :)

I didn't know if and how to add a commit to master to the same PR, so I created a new one. I hope this is ok.

I also slightly tweaked the conditions for core counts and localhost vectors to be more meaningful:

# Numeric workers argument
# previous condition:
if (length(workers_arg) == 1L &&
    is.numeric(workers_arg) &&
    !is.na(workers_arg) &&
    workers_arg >= 0)
{ ...
# current PR:
if (is.numeric(workers_arg) &&
    length(workers_arg) == 1L &&
    !is.na(workers_arg) &&
    workers_arg >= 1)
{ ...


# Character vector of localhosts as workers argument
# previous condition:
if (length(workers_arg) >= 1L &&
    is.character(workers_arg) &&
    all(workers_arg %in% localhosts))
{ ...
# current PR:
if (is.character(workers_arg) &&
    length(workers_arg) >= 1L &&
    all(!is.na(workers_arg)) &&
    all(workers_arg %in% localhosts))
{ ...

@Jean-Romain
Copy link
Collaborator

I didn't know if and how to add a commit to master to the same PR, so I created a new one. I hope this is ok.

Me neither. I think I could have done that myself in command line with git locally. Anyway it worked.

I also slightly tweaked the conditions for core counts and localhost vectors to be more meaningful:

It seems better indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in the package Enhancement Not actually a bug but a possible improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants