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

Proposition: Change meta (add get(nlp, :thing)) #92

Closed
abelsiqueira opened this issue Jul 12, 2017 · 6 comments
Closed

Proposition: Change meta (add get(nlp, :thing)) #92

abelsiqueira opened this issue Jul 12, 2017 · 6 comments

Comments

@abelsiqueira
Copy link
Member

Instead of using nlp.meta.nvar, use get(nlp, :nvar) or getfield.

In particular, a related problem is in the NLS #91 nls_meta. Since we don't have OO, we can't have nls_meta extending meta only including nequ. With the proposed change, though, we can define

abstract AbstractNLSMeta
type NLSMeta <: AbstractNLSMeta
  meta :: AbstractNLPMeta
  nequ :: Int
end
abstract AbstractNLSModel
type LLSModel <: AbstractNLSModel
  meta :: NLSMeta
  A :: Matrix
  b :: Vector
end

meta(nlp :: AbstractNLPModel) = nlp.meta
meta(nls :: AbstractNLSModel) = nls.meta.meta
get(nlp :: AbstractNLPModel, name :: Symbol) = getfield(meta(nlp), name)
@dpo
Copy link
Member

dpo commented Jul 13, 2017

We could probably use an overarching AbstractMeta. Using metaprogramming, we can probably define methods

nvar(model::AbstractNLPModel) = model.meta.nvar
ncon(model::AbstractNLPModel) = model.meta.nvar
...

which may look more intuitive than get()!?

@abelsiqueira
Copy link
Member Author

Yes, this also works for my use case. I'll try this change on the nls branch, and if it looks good I'll refactor.

@abelsiqueira
Copy link
Member Author

What I missed before is that these names are fairly common choices for variables, and thye would conflict.

nvar = nvar(nlp)
...
nvar(nlp) # error in the same scope

On the other hand, usually you would either have a single nvar, so no need to call nvar(nlp) again, or multiple nvar, in which case, you'd have to name it better.
The only really conflicting use case I found was

function FeasModel(nlp :: AbstractNLPModel; name::String = name(nlp))
  ...
end

in which case, name::String had to be changed.

Do you see this becoming a problem?

@abelsiqueira
Copy link
Member Author

I was wrong, nvar = nvar(nlp) won't work inside a function.

foo(a) = a+1
function bar()
  foo = foo(2)
end
bar() # ERROR: UndefVarError: foo not defined

I think the best approach here would be create a descriptive function call, e.g. nvar = number_of_variables(nlp). But then, the meta cleanup should be done first.

@dpo
Copy link
Member

dpo commented Aug 26, 2017

How about get_nvar()? There's a long discussion here: JuliaLang/julia#16770

@abelsiqueira
Copy link
Member Author

Not implementing as discussed in #237.

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

Successfully merging a pull request may close this issue.

2 participants