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

Ignore indexes combinations that do not exist #202

Closed
hugolarzabal opened this issue Jun 15, 2018 · 5 comments
Closed

Ignore indexes combinations that do not exist #202

hugolarzabal opened this issue Jun 15, 2018 · 5 comments

Comments

@hugolarzabal
Copy link
Contributor

Hi Dirk,

I have been using ompr to solve some large optimization problems and I really like the way to model variables and constraints that it provides.

However, I had some troubles with variables with multiple indexes and combinations that do not exist.
As an example, I will take a variable defined with 4 indexes from 1 to 100 each.

MILPModel() %>% add_variable ( x[i,j,k,l], i =1:100, j =1:100, k =1:100, l = 1:100, filter = my_filter )

The complete combination of the indexes would create 100 millions combinations but with the filter only 10000 (for example) combinations are allowed to exist. Using the filter here is absolutely necessary. Without it, the model will be really big and slow.

The tricky part comes when I try to make a sum_expr over some indexes:

 %>% add_constraint( sum_expr( x[i, j, k,l], i= 1:100, j = 1:100, filter = filter??? ) <= 1, k = 1:100, l =1:100 ) 

Here, creating the filter in the sum_expr can be a true nightmare. In some case, I didn't find a way to do it.

I think that instead of having to put filters in every new constraints, the function could simply ignore combinations that do not exist. That would make modelling a lot easier.

Looking into the code, I found the part responsible for checking that the combinations exist in linearopt-variables.R in line 540.

 cols <- merge(new_indexes, index_mapping, by = join_cols)
  if (nrow(cols) != nrow(new_indexes)) {
    stop("You used the variable '", var_name, "' with at least one index that does not exists.", call. = FALSE)
  }

From what I understand, the function filters out (by merging them with the index mapping) the combination of indexes that have not been declared and gives an error if some combinations have been eliminated that way.

I tried changing the stop with a simple warning: the constraint is created only with the existing combinations and ignore the ones that do not exist.
I tried it with large models : it didn't produce any error and gave correct results.

Do you think that this change could be implemented in the library ? I think it would make large and complex models a lot easier to create.

@dirkschumacher
Copy link
Owner

👋
These are very valid points and I appreciate your feedback - thanks a lot!👍 I think about it, but I cannot promise a time frame.

I particular like this:

I think that instead of having to put filters in every new constraints, the function could simply ignore combinations that do not exist. That would make modelling a lot easier.

@hugolarzabal
Copy link
Contributor Author

thank you. That will be great if this change is implemented.

I think the only change that you need to do is at line 541 of * linearopt-variables.R*.

Replace stop

stop("You used the variable '", var_name, "' with at least one index that does not exists.", call. = FALSE)

by warning

warning("You used the variable '", var_name, "' with at least one index that does not exists.", call. = FALSE)

The rest of the code already ignore indexes that do not exist. I tried this change and it worked without any problems with diferent models.

Maybe the warning message could be improved to give more informations and allow the user to understand if he has used indexes that do not exist by mistake of because he doesn't want to put a filter.

Anyway, I understand that it will take you sometime to try and test it.

@rickyars
Copy link

@hugolarzabal have you thought about doing a pull request? It shouldn't be too hard since you've already identified the fix and it seems to be pretty easy. And since you aren't modifying outputs, like I was, you probably won't need to mess with the tests.

@hugolarzabal
Copy link
Contributor Author

I have made a pull request with the change.

Since it is the first time I make a pull request in github, please tell me if I forgot anything.

@dirkschumacher
Copy link
Owner

Fixed #203

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

No branches or pull requests

3 participants