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

getObjective method implemented. #592

Closed
wants to merge 3 commits into from
Closed

getObjective method implemented. #592

wants to merge 3 commits into from

Conversation

Vgrunert
Copy link

No description provided.

@@ -199,6 +199,8 @@ end
@Base.deprecate getNumVars(m::Model) MathProgBase.numvar(m)
@Base.deprecate getNumConstraints(m::Model) MathProgBase.numlinconstr(m)

getCost(m::Model) = m.redCosts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already available through getDual(::Variable)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. The getDual returns the costs of one variable, whereas the redCosts returns the costs associated with the entire model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n = MathProgBase.numvar(model)
z = zeros(n)
for i in 1:n
    z[i] = getDual(Variable(model,i))
end

Accomplishes this using the existing documented methods.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. You decide of course. I just thought that it's a nice convenience function. But please consider the getObjective function, as it is being exported but not implemented so far.

@mlubin
Copy link
Member

mlubin commented Sep 28, 2015

Could you describe a bit what you're trying to do and why these new methods are needed?

@Vgrunert
Copy link
Author

Yes. The getObjective function was added simply because it was not implemented so far. I tried to use it but could not, so I added it. The getCost function is useful to get an overview of the reduced costs / shadow costs of the entire model, compared to the getDual function which provides the costs of a particular variable.

@mlubin
Copy link
Member

mlubin commented Sep 28, 2015

Yes, exported but not implemented is a bit bizarre, not sure how that happened. @IainNZ

@Vgrunert
Copy link
Author

Well Thanks for your consideration. I was also thinking about adding a few docstrings in the near future, so until then all the best.

@mlubin
Copy link
Member

mlubin commented Sep 28, 2015

I think it's a good idea to provide getObjective, along with way to access constraints (#462)

@IainNZ
Copy link
Collaborator

IainNZ commented Sep 28, 2015

Yes to getObjective, no to getCost, docstrings is good but lets hold off for now until we think about style.

@Vgrunert Vgrunert closed this Sep 29, 2015
@Vgrunert Vgrunert reopened this Sep 29, 2015
@joehuchette
Copy link
Contributor

I think we should merge da361e5

@mlubin
Copy link
Member

mlubin commented Oct 6, 2015

Needs docs and tests and handling for nonlinear objectives

@Vgrunert
Copy link
Author

Vgrunert commented Oct 7, 2015

Sry, I am not familiar with github. Is there anything on my part that needs to be done? I though that the request is already out and has to be accepted.

@mlubin
Copy link
Member

mlubin commented Oct 7, 2015

You can make changes to your PR at any time by modifying the corresponding branch on your fork (in this case master). If you're up to it you could tidy the PR by removing the getCost changes and adding documentation for getObjective, otherwise we'll do it when we get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants