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

Implement quadratic functionality #802

Merged
merged 28 commits into from
Dec 10, 2023
Merged

Implement quadratic functionality #802

merged 28 commits into from
Dec 10, 2023

Conversation

stelmo
Copy link
Collaborator

@stelmo stelmo commented Nov 11, 2023

No description provided.

@exaexa exaexa force-pushed the sew-fba-tests-etc branch 2 times, most recently from f0c6678 to fe1ef35 Compare December 7, 2023 20:31
Base automatically changed from sew-fba-tests-etc to next December 7, 2023 20:44
@exaexa
Copy link
Collaborator

exaexa commented Dec 8, 2023

/format

Copy link
Contributor

github-actions bot commented Dec 8, 2023

✔️ Auto-formatting triggered by this comment succeeded, commited as e966427

github-actions bot pushed a commit that referenced this pull request Dec 8, 2023
triggered by @exaexa on PR #802
@exaexa exaexa force-pushed the sew-pfba-tests-etc branch from e966427 to a985ffa Compare December 9, 2023 15:37
@exaexa exaexa mentioned this pull request Dec 9, 2023
@exaexa exaexa force-pushed the sew-pfba-tests-etc branch from 00e2f41 to 0bc99a2 Compare December 9, 2023 22:01
@exaexa exaexa force-pushed the sew-pfba-tests-etc branch from 0bc99a2 to a2c121e Compare December 9, 2023 22:04
@exaexa
Copy link
Collaborator

exaexa commented Dec 9, 2023

@stelmo if you could have a quick look now it would be great. I've set up proper intermediate (CT-based) functions for FBA/pFBA; same will follow with MOMA and FVA and likely later for others. These will also be used exclusively for the complex models.

I split off the MOMA from QP tutorial. To be more realistic I assume we generally want to run pFBA instead of plain FBA to obtain a sane initial "reference" solution, right?

Copy link
Collaborator Author

@stelmo stelmo left a comment

Choose a reason for hiding this comment

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

Small things

ctmodel;
objective = ctmodel.:l2objective.value,
optimizer = Clarabel.Optimizer,
sense = Minimal,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like this Minimal. Since we are using JuMP's set_optimizer_attribute etc., we should use JuMP's Min and Max

Copy link
Collaborator

Choose a reason for hiding this comment

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

we're renaming JuMP's symbols to ours (people don't import them from JuMP!) so we shouldn't also force them to import parameters.


vt = C.constraint_values(ctmodel, J.value.(opt_model[:x])) # ConstraintTrees.jl is called C in COBREXA

@test isapprox(vt.l2objective, ?; atol = QP_TEST_TOLERANCE) #src # TODO will break until mutable bounds
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can remove TODO

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still commented out :D


vt = C.constraint_values(ctmodel, J.value.(opt_model[:x])) # ConstraintTrees.jl is called C in COBREXA

@test isapprox(vt.l2objective, ?; atol = QP_TEST_TOLERANCE) #src # TODO will break until mutable bounds
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can remove TODO

Change the objective sense of optimization. Possible arguments are
`JuMP.MAX_SENSE` and `JuMP.MIN_SENSE`.
Change the objective sense of optimization. Accepted arguments include
[`Minimal`](@ref), [`Maximal`](@ref), and [`Feasible`](@ref).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What does Feasible do?

Copy link
Collaborator

Choose a reason for hiding this comment

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


Same as `JuMP.FEASIBILITY_SENSE`.
"""
const Feasible = J.FEASIBILITY_SENSE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did not know this exists

Comment on lines +50 to +75

"""
Minimal

Objective sense for finding the minimal value of the objective.

Same as `JuMP.MIN_SENSE`.
"""
const Minimal = J.MIN_SENSE

"""
Maximal

Objective sense for finding the maximal value of the objective.

Same as `JuMP.MAX_SENSE`.
"""
const Maximal = J.MAX_SENSE

"""
Maximal

Objective sense for finding the any feasible value of the objective.

Same as `JuMP.FEASIBILITY_SENSE`.
"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My concern here is that it is just more stuff to remember. Can we not just re-export the JuMP constants?

Copy link
Collaborator

Choose a reason for hiding this comment

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

one can use the jump constants as well. I just don't want to 1] cause identifier collision 2] have folks to import jump all over again everytime they want to do something

@stelmo
Copy link
Collaborator Author

stelmo commented Dec 10, 2023

@stelmo if you could have a quick look now it would be great. I've set up proper intermediate (CT-based) functions for FBA/pFBA; same will follow with MOMA and FVA and likely later for others. These will also be used exclusively for the complex models.

I split off the MOMA from QP tutorial. To be more realistic I assume we generally want to run pFBA instead of plain FBA to obtain a sane initial "reference" solution, right?

Yep, it would make sense to use a pFBA solution for MOMA, but as long as its general i.e. the user can decide how sane they want to be, I don't mind

@exaexa
Copy link
Collaborator

exaexa commented Dec 10, 2023

Yep, it would make sense to use a pFBA solution for MOMA, but as long as its general i.e. the user can decide how sane they want to be, I don't mind

I converted this to an admonition opportunity.

@exaexa exaexa merged commit efaea20 into next Dec 10, 2023
1 check passed
@exaexa exaexa deleted the sew-pfba-tests-etc branch December 10, 2023 14:38
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

Successfully merging this pull request may close these issues.

2 participants