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

Latex printer #2984

Merged
merged 42 commits into from
Nov 27, 2023
Merged

Latex printer #2984

merged 42 commits into from
Nov 27, 2023

Conversation

codykarcher
Copy link
Contributor

Fixes # 752

#752

Summary/Motivation:

Latex is good, Pyomo is good, together are great

Changes proposed in this PR:

Adds a util function that prints out the latex of various pyomo objects. Docs and unit tests have also been included

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

I warned you in advance of all the comments... ;)

doc/OnlineDocs/model_debugging/latex_printing.rst Outdated Show resolved Hide resolved
pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
pyomo/util/latex_printer.py Outdated Show resolved Hide resolved


def generate_simple_model():
import pyomo.environ as pe
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to re-import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to have the generate model function be copy-paste-able and believe this import is of minimal overhead



def generate_simple_model_2():
import pyomo.environ as pe
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to re-import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to have the generate model function be copy-paste-able and believe this import is of minimal overhead

Comment on lines 17 to 124
m.z = pe.Var()
m.objective_1 = pe.Objective(expr=m.x + m.y + m.z)
m.constraint_1 = pe.Constraint(
expr=m.x**2 + m.y**-2.0 - m.x * m.y * m.z + 1 == 2.0
)
m.constraint_2 = pe.Constraint(expr=abs(m.x / m.z**-2) * (m.x + m.y) <= 2.0)
m.constraint_3 = pe.Constraint(expr=pe.sqrt(m.x / m.z**-2) <= 2.0)
m.constraint_4 = pe.Constraint(expr=(1, m.x, 2))
m.constraint_5 = pe.Constraint(expr=Expr_if(m.x <= 1.0, m.z, m.y) <= 1.0)

def blackbox(a, b):
return sin(a - b)

m.bb = ExternalFunction(blackbox)
m.constraint_6 = pe.Constraint(expr=m.x + m.bb(m.x, m.y) == 2)

m.I = pe.Set(initialize=[1, 2, 3, 4, 5])
m.J = pe.Set(initialize=[1, 2, 3])
m.K = pe.Set(initialize=[1, 3, 5])
m.u = pe.Var(m.I * m.I)
m.v = pe.Var(m.I)
m.w = pe.Var(m.J)
m.p = pe.Var(m.K)

m.express = pe.Expression(expr=m.x**2 + m.y**2)

def ruleMaker(m, j):
return (m.x + m.y) * sum(m.v[i] + m.u[i, j] ** 2 for i in m.I) <= 0

m.constraint_7 = pe.Constraint(m.I, rule=ruleMaker)

def ruleMaker(m):
return sum(m.p[k] for k in m.K) == 1

m.constraint_8 = pe.Constraint(rule=ruleMaker)

def ruleMaker(m):
return (m.x + m.y) * sum(m.w[j] for j in m.J)

m.objective_2 = pe.Objective(rule=ruleMaker)

m.objective_3 = pe.Objective(expr=m.x + m.y + m.z, sense=-1)

return m


def generate_simple_model():
import pyomo.environ as pe

m = pe.ConcreteModel(name='basicFormulation')
m.x = pe.Var()
m.y = pe.Var()
m.objective_1 = pe.Objective(expr=m.x + m.y)
m.constraint_1 = pe.Constraint(expr=m.x**2 + m.y**2.0 <= 1.0)
m.constraint_2 = pe.Constraint(expr=m.x >= 0.0)

m.I = pe.Set(initialize=[1, 2, 3, 4, 5])
m.J = pe.Set(initialize=[1, 2, 3])
m.K = pe.Set(initialize=[1, 3, 5])
m.u = pe.Var(m.I * m.I)
m.v = pe.Var(m.I)
m.w = pe.Var(m.J)
m.p = pe.Var(m.K)

def ruleMaker(m, j):
return (m.x + m.y) * sum(m.v[i] + m.u[i, j] ** 2 for i in m.I) <= 0

m.constraint_7 = pe.Constraint(m.I, rule=ruleMaker)

def ruleMaker(m):
return sum(m.p[k] for k in m.K) == 1

m.constraint_8 = pe.Constraint(rule=ruleMaker)

return m


def generate_simple_model_2():
import pyomo.environ as pe

m = pe.ConcreteModel(name='basicFormulation')
m.x_dot = pe.Var()
m.x_bar = pe.Var()
m.x_star = pe.Var()
m.x_hat = pe.Var()
m.x_hat_1 = pe.Var()
m.y_sub1_sub2_sub3 = pe.Var()
m.objective_1 = pe.Objective(expr=m.y_sub1_sub2_sub3)
m.constraint_1 = pe.Constraint(
expr=(m.x_dot + m.x_bar + m.x_star + m.x_hat + m.x_hat_1) ** 2
<= m.y_sub1_sub2_sub3
)
m.constraint_2 = pe.Constraint(
expr=(m.x_dot + m.x_bar) ** -(m.x_star + m.x_hat) <= m.y_sub1_sub2_sub3
)
m.constraint_3 = pe.Constraint(
expr=-(m.x_dot + m.x_bar) + -(m.x_star + m.x_hat) <= m.y_sub1_sub2_sub3
)

return m
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be inside the test class, FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer them here, but would be willing to move if others feel strongly.

pyomo/util/tests/test_latex_printer.py Outdated Show resolved Hide resolved
pyomo/util/tests/test_latex_printer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

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

@codykarcher, thanks so much for doing this (and at the speed of light too!).

Some design questions around how we name variables, and a lot of small comments. I haven't actually played with it yet, so there's probably more coming. :)

doc/OnlineDocs/model_debugging/latex_printing.rst Outdated Show resolved Hide resolved
:type filename: str
:param useAlignEnvironment: If False, the equation/aligned construction is used to create a single LaTeX equation. If True, then the align environment is used in LaTeX and each constraint and objective will be given an individual equation number
:type useAlignEnvironment: bool
:param splitContinuousSets: If False, all sums will be done over 'index in set' or similar. If True, sums will be done over 'i=1' to 'N' or similar if the set is a continuous set
Copy link
Contributor

Choose a reason for hiding this comment

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

If the set is a continuous set, we shouldn't be summing over it at all, should we? Where does this case come up?

Comment on lines 24 to 30
.. note::

If operating in a Jupyter Notebook, it may be helpful to use:

``from IPython.display import display, Math``

``display(Math(latex_printer(m))``
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a cool note to include!


The LaTeX printer will auto detect the following structures in variable names:

* ``_``: underscores will get rendered as subscripts, ie ``x_var`` is rendered as ``x_{var}``
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected behavior for this_is_a_very_descriptive_variable_name? I ask because we encourage users to be more PEP8 than math when they write models: that is long descriptive names rather than m.x. So I'm actually not exaggerating too badly. I'm wondering if it might be easier to just generate variable names for the latex and return a map of the original var to the generated one. Or, even better, maybe the user could optionally provide their own map that you would use... I think I would like that option the best.

Comment on lines 35 to 37
* ``_dot``: will format as a ``\dot{}`` and remove from the underscore formatting. Ex: ``x_dot_1`` becomes ``\dot{x}_1``
* ``_hat``: will format as a ``\hat{}`` and remove from the underscore formatting. Ex: ``x_hat_1`` becomes ``\hat{x}_1``
* ``_bar``: will format as a ``\bar{}`` and remove from the underscore formatting. Ex: ``x_bar_1`` becomes ``\bar{x}_1``
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these make me be in favor of the map idea too. These are nice, but the problem is where do we stop. Why not mathcal or underline or the list goes on...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smart variables are no longer the default behavior, and will implement a mapping as requested

pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
if len(constraints) > 0:
# only print this if printing a full formulation
if not isSingle:
pstr += ' ' * tbSpc + '& \\text{subject to} \n'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer \text{s.t.} in the latex here

'''

# Various setup things
isSingle = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename this variable to be singleEquation or something like that then?

Comment on lines 489 to 503
setPreferenceOrder = [
'I',
'J',
'K',
'M',
'N',
'P',
'Q',
'R',
'S',
'T',
'U',
'V',
'W',
]
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you run out of names in this list?

Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

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

A few more thoughts after actually playing around with it this morning...

pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
pyomo/util/latex_printer.py Outdated Show resolved Hide resolved
newVariableList = ['x' for i in range(0, len(variableList))]
for i in range(0, len(variableList)):
newVariableList[i] += '_' + str(i + 1)
overwriteDict = dict(zip([v.name for v in variableList], newVariableList))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this dictionary should really be a ComponentMap and that the keys should by pyomo components (rather than their names). Also, do you think we could support naming Sets and Params out of it as well? Sets are the hard ones because there's what you call the set, and then what you use for indices. But perhaps to start we could have people specify a list of index names and if you run out you can just yell at them. Thinking of cases like:

@m.Constraint(m.TIME)
def something(m, t):
    return sum(m.some_var[t_prime] for t_prime in m.TIME if t_prime < t) == 1

You'd want overwriteDict[m.TIME] = ('\mathcal T', ['t', "t'"]) or something like that...

indices[0]._set,
)

conLine += ', \\quad %s \\in %s ' % (idxTag, setTag)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a \forall before idxTag?

Comment on lines 721 to 723
# close off the print string
if useAlignEnvironment:
pstr += '\\end{align} \n'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also keep track of what Vars were used in the Constraints/Objectives we wrote and write the variable domains after the Constraints. (I'm thinking this feels like expected behavior when you're writing a whole Block or model.)

@fleimgruber
Copy link

@codykarcher As an early subscriber to #752 I was happy to see this attempt upstream! In the meantime we rolled our own internal version of this and use it on and off. So of course I was curious and tried this PR with one of our test cases (never mind the actually infeasible and silly model) and saw this error (below test case), hope it helps:

import pyomo.environ as pyo
from pyomo.util.latex_printer import latex_printer


def create_model() -> pyo.ConcreteModel:
    model = pyo.ConcreteModel("Example Model")

    model.years = pyo.Set(initialize=[2030, 2040])
    model.strings = pyo.Set(initialize=["a", "b"])

    model.p = pyo.Param(model.years, initialize={2030: 1, 2040: 2}, mutable=False)
    model.pp = pyo.Param(model.strings, model.years, initialize={("a", 2030): 1, ("b", 2040): 2})

    model.x_etc = pyo.Var(model.years, domain=pyo.NonNegativeReals)

    def obj(m):
        return sum(
            m.p[j_st] * m.pp["a", j_st] * m.x_etc[j_st] for j_st in m.years if j_st == m.p[2030] and j_st != 2030
        ) - (m.p[2040] if m.p[2030] not in [1] else 0)

    model.OBJ = pyo.Objective(rule=obj)

    return model


latex_printer(create_model())

Error:

Traceback (most recent call last):
  File "/home/user/dev/pyomo_latex_printer/latex_printer_upstream.py", line 35, in <module>
    latex_printer(model)
  File "/home/user/dev/pyomo/pyomo/util/latex_printer.py", line 671, in latex_printer
    obj_template, obj_indices = templatize_fcn(obj)
                                ^^^^^^^^^^^^^^^^^^^
  File "/home/user/dev/pyomo/pyomo/core/expr/template_expr.py", line 1178, in templatize_constraint
    expr, indices = templatize_rule(con.parent_block(), con.rule, con.index_set())
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/dev/pyomo/pyomo/core/expr/template_expr.py", line 1166, in templatize_rule
    "templatizing the rule '%s':\n\t%s" % (rule.name, internal_error[1])
                                           ^^^^^^^^^
AttributeError: 'ScalarCallInitializer' object has no attribute 'name'

@mrmundt
Copy link
Contributor

mrmundt commented Sep 7, 2023

@fleimgruber - that is insidious. That is an error that occurs while trying to raise a separate error. Thanks for being an early tester!

@mrmundt
Copy link
Contributor

mrmundt commented Sep 7, 2023

@fleimgruber - I ran your code (with a silly workaround to get past the initial error), and this is what prints out:

PyomoException: Cannot convert non-constant Pyomo expression (_1  ==  1) to bool.
This error is usually caused by using a Var, unit, or mutable Param in a
Boolean context such as an "if" statement, or when checking container
membership or equality. For example,
    >>> m.x = Var()
    >>> if m.x >= 1:
    ...     pass
and
    >>> m.y = Var()
    >>> if m.y in [m.x, m.y]:
    ...     pass
would both cause this exception.

During handling of the above exception, another exception occurred:

TemplateExpressionError                   Traceback (most recent call last)
Cell In[1], line 26
     21     model.OBJ = pyo.Objective(rule=obj)
     23     return model
---> 26 latex_printer(create_model())

File ~/Documents/idaes/cody-pyomo/pyomo/util/latex_printer.py:671, in latex_printer(pyomo_component, filename, use_align_environment, split_continuous_sets, use_smart_variables, x_only_mode, use_short_descriptors, use_forall, overwrite_dict)
    669 # Iterate over the objectives and print
    670 for obj in objectives:
--> 671     obj_template, obj_indices = templatize_fcn(obj)
    672     if obj.sense == 1:
    673         pstr += ' ' * tbSpc + '& %s \n' % (descriptorDict['minimize'])

File ~/Documents/idaes/cody-pyomo/pyomo/core/expr/template_expr.py:1178, in templatize_constraint(con)
   1177 def templatize_constraint(con):
-> 1178     expr, indices = templatize_rule(con.parent_block(), con.rule, con.index_set())
   1179     if expr.__class__ is tuple:
   1180         expr = tuple_to_relational_expr(expr)

File ~/Documents/idaes/cody-pyomo/pyomo/core/expr/template_expr.py:1168, in templatize_rule(block, rule, index_set)
   1163         if internal_error is not None:
   1164             logger.error(
   1165                 "The following exception was raised when "
   1166                 "templatizing the rule '%s':\n\t%s" % (rule, internal_error[1])
   1167             )
-> 1168         raise TemplateExpressionError(
   1169             None,
   1170             "Explicit iteration (for loops) over Sets is not supported "
   1171             "by template expressions.  Encountered loop over %s"
   1172             % (context.cache[-1][0]._set,),
   1173         )
   1174 return None, indices

TemplateExpressionError: Explicit iteration (for loops) over Sets is not supported by template expressions.  Encountered loop over years

This is an issue in how the latex_printer iterates, so thanks for the info! It exposes a real problem on our end. We appreciate it.

@codykarcher
Copy link
Contributor Author

@jsiirola I think this bug needs a quick look from you to triage if its in my code or your code

@fleimgruber
Copy link

@mrmundt happy to hear it helped!

@codykarcher
Copy link
Contributor Author

@fleimgruber Would you mind posting the expected latex output of your test case? That way I can verify my fixes.

@codykarcher codykarcher changed the title Latex printer [WIP] Latex printer Sep 7, 2023
@codykarcher codykarcher marked this pull request as ready for review October 12, 2023 18:12
@emma58 emma58 self-requested a review October 31, 2023 19:01
@mrmundt mrmundt self-requested a review November 7, 2023 19:40
Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

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

@codykarcher, a lot more comments, but most of them are minor. My only design-related thought is that I think we could still reduce the number of options since the global formatting for latex documents (like margins and fontsize) is very easy to change after the fact. That will reduce some complexity in your option parsing especially.


.. py:function:: latex_printer(pyomo_component, latex_component_map=None, write_object=None, use_equation_environment=False, split_continuous_sets=False, use_short_descriptors=False, fontsize = None, paper_dimensions=None)

Prints a pyomo element (Block, Model, Objective, Constraint, or Expression) to a LaTeX compatible string
Copy link
Contributor

Choose a reason for hiding this comment

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

"pyomo element" should be "pyomo component" (just for the sake of consistency)

:type latex_component_map: pyomo.common.collections.component_map.ComponentMap
:param write_object: The object to print the latex string to. Can be an open file object, string I/O object, or a string for a filename to write to
:type write_object: io.TextIOWrapper or io.StringIO or str
:param use_equation_environment: If False, the equation/aligned construction is used to create a single LaTeX equation. If True, then the align environment is used in LaTeX and each constraint and objective will be given an individual equation number
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing to read... I wonder if it would be clearer to call this use_aligned_environment? If this flag is False and I give a whole model, do I just get a series of \begin{equation} ... \end{equation} blocks for every objective/constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to have the flag flip true for non-default behavior, but will attempt to make the note more clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also had the explanation backwards

:type write_object: io.TextIOWrapper or io.StringIO or str
:param use_equation_environment: If False, the equation/aligned construction is used to create a single LaTeX equation. If True, then the align environment is used in LaTeX and each constraint and objective will be given an individual equation number
:type use_equation_environment: bool
:param split_continuous_sets: If False, all sums will be done over 'index in set' or similar. If True, sums will be done over 'i=1' to 'N' or similar if the set is a continuous set
Copy link
Contributor

Choose a reason for hiding this comment

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

This name also confuses me because mathematically a "continuous set" isn't a thing. I think I would describe what you means as a 'contiguous' set. Maybe this argument should be explicit_summation_index_for_contiguous_sets or something like that (but shorter :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I'll mull this and do something better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went with explicit_set_summation Maybe better, maybe not

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, definitely better! I like it.

:type use_equation_environment: bool
:param split_continuous_sets: If False, all sums will be done over 'index in set' or similar. If True, sums will be done over 'i=1' to 'N' or similar if the set is a continuous set
:type split_continuous_sets: bool
:param use_short_descriptors: If False, will print full 'minimize' and 'subject to' etc. If true, uses 'min' and 's.t.' instead
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is easy enough to change manually that we probably don't need to bother with this argument? I would obviously vote for the short versions, but I'll live if I get outvoted. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine Emma, you win :P I'll remove the long form and the flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'm putting a back door in the component dict so I can still make it do what I want

:type split_continuous_sets: bool
:param use_short_descriptors: If False, will print full 'minimize' and 'subject to' etc. If true, uses 'min' and 's.t.' instead
:type use_short_descriptors: bool
:param fontsize: Sets the font size of the latex output when writing to a file. Can take in any of the latex font size keywords ['tiny', 'scriptsize', 'footnotesize', 'small', 'normalsize', 'large', 'Large', 'LARGE', 'huge', 'Huge'], or an integer referenced off of 'normalsize' (ex: small is -1, Large is +2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems easy enough to change manually that it doesn't need to be an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's convenient, but I buy your point. Will change

Comment on lines 600 to 601
# else:
# lowerBound = ' 0 \\leq '
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need these comments?

Comment on lines 569 to 570
# else:
# lowerBound = ' 0 \\leq '
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need these comments?

Comment on lines 560 to 561
# else:
# upperBound = ' < 0 '
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need these comments?

Comment on lines 541 to 542
# else:
# upperBound = ' \\leq 0 '
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you don't need these comments?

Comment on lines 515 to 517
raise ValueError(
'Formulation is infeasible due to bounds on variable %s' % (vr.name)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

There actually is an InfeasibleConstraintException in pyomo.common.errors that might be the right thing here (and in place of the ValueErrors below also).

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (ae85c0c) 87.98% compared to head (c29439e) 88.06%.
Report is 19 commits behind head on main.

Files Patch % Lines
pyomo/contrib/latex_printer/latex_printer.py 92.87% 48 Missing ⚠️
pyomo/contrib/latex_printer/__init__.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2984      +/-   ##
==========================================
+ Coverage   87.98%   88.06%   +0.07%     
==========================================
  Files         770      773       +3     
  Lines       90242    91262    +1020     
==========================================
+ Hits        79400    80368     +968     
- Misses      10842    10894      +52     
Flag Coverage Δ
linux 85.41% <92.62%> (+0.10%) ⬆️
osx 75.34% <92.62%> (+0.22%) ⬆️
other 85.59% <92.62%> (+0.10%) ⬆️
win 82.70% <92.62%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@blnicho blnicho left a comment

Choose a reason for hiding this comment

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

I have a few minor comments and suggestions but otherwise I think this is ready to merge.


Pyomo models can be printed to a LaTeX compatible format using the ``pyomo.util.latex_printer`` function:

.. py:function:: latex_printer(pyomo_component, latex_component_map=None, write_object=None, use_equation_environment=False, explicit_set_summation=False, use_short_descriptors=False, fontsize = None, paper_dimensions=None)
Copy link
Member

Choose a reason for hiding this comment

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

Use the Sphinx autofunction command here to automatically render the docstring from the code. This will ensure the documentation is always synced to the implementation.

:type throw_templatization_error: bool


:return: A LaTeX style string that represents the passed in pyomoElement
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:return: A LaTeX style string that represents the passed in pyomoElement
:return: A LaTeX style string that represents the passed in Pyomo component

Comment on lines 79 to 82
from pyomo.common.dependencies import numpy, numpy_available

if numpy_available:
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from pyomo.common.dependencies import numpy, numpy_available
if numpy_available:
import numpy as np
from pyomo.common.dependencies import numpy as np, numpy_available

Comment on lines 44 to 46
import pyomo.environ as pyo
from pyomo.core.expr import Expr_if
from pyomo.core.base import ExternalFunction
Copy link
Member

Choose a reason for hiding this comment

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

Why not just import these at the top of this file with everything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I like copy-pasteable unit tests

Comment on lines 67 to 69
m.I = pyo.Set(initialize=[1, 2, 3, 4, 5])
m.J = pyo.Set(initialize=[1, 2, 3])
m.K = pyo.Set(initialize=[1, 3, 5])
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested the printer on Sets containing non-numeric entries like strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but in theory it shouldn't matter what's in the set unless you use the explicit_set_summation flag. There is a known issue with how things like pro.Set()*pyo.Set() end up rendering, but I'm living with that for now

>>> m.objective = pyo.Objective( expr = m.x + m.y + m.z )
>>> m.constraint_1 = pyo.Constraint(expr = m.x**2 + m.y**2.0 - m.z**2.0 <= m.c )

>>> pstr = latex_printer(m)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to not suppress the result here and instead include the output (it both tests that the output doesn't change, and documents the output for the user). Same for the other examples below...

if math.log(num, base).is_integer():
numDigs += 1

digs = [0.0 for i in range(0, numDigs)]
Copy link
Member

Choose a reason for hiding this comment

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

Why initialize to floating point? Shouldn't this all be integer math?

import numpy as np


def decoder(num, base):
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessarily complex. Shouldn't the following be simpler:

def decoder(num, base):
    if int(num) != abs(num):
        # Requiring an integer is nice, but not strictly necessary;
        # the algorithm works for floating point
        raise ValueError("num should be a nonnegative integer")
    if int(base) != abs(base) or not base:
        raise ValueError("base should be a positive integer")
    ans = []
    while 1:
        ans.append(num % base)
        num //= base
        if not num:
            return reversed(ans)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand this code, but it worked so going with it

Comment on lines 160 to 166
if hasattr(node, 'PRECEDENCE'):
precedence = node.PRECEDENCE
else:
# Should never hit this
raise DeveloperError(
'This error should never be thrown, node does not have a precedence. Report to developers'
)
Copy link
Member

Choose a reason for hiding this comment

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

If this is a DeveloperError, why not just precedence = node.PRECEDENCE and live with the resulting AttributeError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used developer error for explicitness.

)

if childPrecedence[0] > precedence:
arg1 = ' \\left( ' + arg1 + ' \\right) '
Copy link
Member

Choose a reason for hiding this comment

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

It might be easier to write these as raw strings so we don't have to escape so many backslashes; e.g.

arg1 = r' \left( ' + arg1 + r' \right) '

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possibly, but this was the bane of my existence for like 3 days. If there is no performance improvement, it's staying as is for now

Comment on lines 559 to 562
else:
raise DeveloperError(
'Invalid domain somehow encountered, contact the developers'
)
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 actually easy to hit. Variable domains can be any valid Pyomo Set, so the following is perfectly legitimate:

m = ConcreteModel()
m. v_domain = Set(initialize=[3,4,5,6])
m.x = Var(domain=m.v_domain)

The thing to use to infer domains are the Var is_continuous(), is_integer(), and is_is_binary() methods:

>>> m.x.is_integer()
True
>>> m.x.is_binary()
False
>>> m.x.is_continuous()
False
>>> m.x.bounds
(3, 6)

For the LP / NL writers, we only require that one of those three return True. So, for example, you can write a model with a domain [1,2,3,7,8,9], but the solver interfaces will complain (because it is not a bounded contiguous integer domain)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to a NotImplementedError for the time being

return pattern.sub(lambda x: rep_dict[x.group(0)], pstr)


def latex_printer(
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 a HUGE function, and we should look at breaking it up into helper functions

setList = []

else:
variableList = [
Copy link
Member

Choose a reason for hiding this comment

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

We should reconsider this mode. The writers have been moving away from pre-gathering variables / parameters / sets and instead just recording the variables as they are encountered when walking expressions. In part, this is because we are relaxing where a Var has to exist (in particular, it doesn't have to exist within the block where is it used).

def latex_printer(
pyomo_component,
latex_component_map=None,
write_object=None,
Copy link
Member

Choose a reason for hiding this comment

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

We generally refer to this as ostream. It would probably be best to maintain consistency.

Comment on lines 622 to 623
str
A LaTeX string of the pyomo_component
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I am in favor of this method always returning a big string. Maybe only return the big string if the user didn't provide an ostream?

This is a special case of a concern I have with the bulk of the method below where we are building up a single HUGE string. Because Python strings are immutable, every time you +=, you are allocating a new string that is a copy of the old one with the extra characters. This leads to "O(n^2)" behavior that can cause this to be very slow for large models. I would rather this be structured to build up (and export) smaller chunks as it goes.


.. py:function:: latex_printer(pyomo_component, latex_component_map=None, write_object=None, use_equation_environment=False, explicit_set_summation=False, use_short_descriptors=False, fontsize = None, paper_dimensions=None)

Prints a pyomo component (Block, Model, Objective, Constraint, or Expression) to a LaTeX compatible string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Prints a pyomo component (Block, Model, Objective, Constraint, or Expression) to a LaTeX compatible string
Prints a Pyomo component (Block, Model, Objective, Constraint, or Expression) to a LaTeX compatible string


:param pyomo_component: The Pyomo component to be printed
:type pyomo_component: _BlockData or Model or Objective or Constraint or Expression
:param latex_component_map: A map keyed by Pyomo component, values become the latex representation in the printer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:param latex_component_map: A map keyed by Pyomo component, values become the latex representation in the printer
:param latex_component_map: A map keyed by Pyomo component, values become the LaTeX representation in the printer

:type pyomo_component: _BlockData or Model or Objective or Constraint or Expression
:param latex_component_map: A map keyed by Pyomo component, values become the latex representation in the printer
:type latex_component_map: pyomo.common.collections.component_map.ComponentMap
:param write_object: The object to print the latex string to. Can be an open file object, string I/O object, or a string for a filename to write to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:param write_object: The object to print the latex string to. Can be an open file object, string I/O object, or a string for a filename to write to
:param write_object: The object to print the LaTeX string to. Can be an open file object, string I/O object, or a string for a filename to write to

Comment on lines 86 to 100
# Needed in the general case, but not as implemented
# if isinstance(base, float):
# if not base.is_integer():
# raise ValueError('Invalid base')
# else:
# base = int(base)

# Needed in the general case, but not as implemented
# if base <= 1:
# raise ValueError('Invalid base')

# Needed in the general case, but not as implemented
# if num == 0:
# numDigs = 1
# else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need all these comments?

dg = math.floor(rem / base**ix)
rem = rem % base**ix
digs[i] = dg
return digs
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is digs? The variable name doesn't make sense to me.

Boolean,
Binary,
Any,
# AnyWithNone,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Comment on lines 38 to 39
# RealInterval,
# IntegerInterval,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Boolean,
Binary,
Any,
# AnyWithNone,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Comment on lines 37 to 38
# RealInterval,
# IntegerInterval,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

return pattern.sub(lambda x: rep_dict[x.group(0)], pstr)


def latex_printer(
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be broken up into smaller functions and not look so monstrously large.

@blnicho blnicho merged commit ac40ca3 into Pyomo:main Nov 27, 2023
30 checks passed
@codykarcher codykarcher deleted the latex_printer branch November 28, 2023 03:47
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.

7 participants