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

Add active filter to flattener #2643

Merged
merged 27 commits into from
Feb 15, 2023
Merged

Conversation

Robbybp
Copy link
Contributor

@Robbybp Robbybp commented Dec 1, 2022

Fixes #2529.

Summary/Motivation:

If a block has been deactivated, that usually means you want to ignore it when searching the block hierarchy.

Changes proposed in this PR:

  • Add active flag to flatten_dae_components and flatten_components_along_sets that functions similarly to the active flag in component_objects
  • Pass this active flag to component objects so slices with in active parent components are not generated
  • If a Component object is active, do not check individual data objects of a slice for activity. The "activity" of a slice is undefined and we do not attempt to infer it. Consider a slice to match the active flag if any of its data objects match the flag.

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.

@Robbybp Robbybp changed the title [WIP] Flatten active blocks [WIP] Add active filter to flattener Dec 1, 2022
@dallan-keylogic
Copy link

This works as expected on my examples. Here's a test I was using if it makes things easier on your side.

    def test_block_deactivation(self):
        m = self._model1_1d_sets()
        # Deactivating b1 should get rid of both variables directly on it
        # as well as those on the subblock b_s
        m.b.b1.deactivate()
        sets = ComponentSet((m.time,))
        sets_list, comps_list = flatten_components_along_sets(m, sets, Var, active=True)

        S = m.space
        SS = m.space * m.space
        SC = m.space * m.comp
        SSC = m.space * m.space * m.comp

        assert len(sets_list) == 3
        for sets, comps in zip(sets_list, comps_list):
            if len(sets) == 1 and sets[0] is UnindexedComponent_set:
                ref_data = {
                    self._hashRef(Reference(m.v0)),
                }
                assert len(comps) == len(ref_data)
                for comp in comps:
                    self.assertIn(self._hashRef(comp), ref_data)
            elif len(sets) == 1 and sets[0] is m.time:
                ref_data = {
                    # Components indexed only by time;
                    self._hashRef(Reference(m.v1)),
                }
                # Components indexed by time and some other set(s)
                ref_data.update(self._hashRef(Reference(m.v2[:, x])) for x in S)
                ref_data.update(self._hashRef(Reference(m.v3[:, x, j])) for x, j in SC)
                ref_data.update(self._hashRef(Reference(m.b.b2[:, x].v0)) for x in S)
                ref_data.update(self._hashRef(Reference(m.b.b2[:, x].v1[j])) for x, j in SC)
                assert len(comps) == len(ref_data)
                for comp in comps:
                    self.assertIn(self._hashRef(comp), ref_data)
            elif len(sets) == 2 and sets[0] is m.time and sets[1] is m.time:
                ref_data = {
                    self._hashRef(Reference(m.v_tt)),
                }
                ref_data.update(self._hashRef(Reference(m.v_tst[:, x, :])) for x in S)
                ref_data.update(self._hashRef(Reference(m.b.b2[:, x].v2[:, j])) for x, j in SC)
                assert len(comps) == len(ref_data)
                for comp in comps:
                    self.assertIn(self._hashRef(comp), ref_data)
            else:
                raise RuntimeError()

Two remaining questions, one substantial and one trivial: what should default behavior be and is active=None any better than active=False? I think it's more intuitive for components on deactivated blocks to be ignored by default than for them to be included, at least as far as using flatten_dae_components for PETSc is concerned.

@Robbybp
Copy link
Contributor Author

Robbybp commented Dec 1, 2022

Thanks for the code, this will make it easier for me to write the tests. Regarding default behavior, I think active=None should be used to maintain backwards compatibility. This is also the default behavior in component_objects and component_data_objects, which I think it is nice to be consistent with.

@Robbybp
Copy link
Contributor Author

Robbybp commented Dec 5, 2022

In the current implementation, the behavior of the flatten_* methods differs slightly from component_objects when the active flag is passed as using active=False will not prevent the flattening functions from returning variables. Instead it will return only the variables whose parent blocks are all inactive. I believe this is a bug in component_objects as this is inconsistent with its behavior when active=True (see my comment on #443), so will keep the current behavior for flattening methods unless I'm told otherwise.

@Robbybp
Copy link
Contributor Author

Robbybp commented Dec 7, 2022

Just made some updates that actually do try to determine the activity of a slice. I did this because I ran into problems descending into BlockData that didn't match the active arg when choosing the index to descend into for a slice. I now descend into the first index I find that matches the active flag. In doing so, a slice is returned if any data object it defines matches the active flag. I also updated the handling for ActiveComponents to be consistent with this.

I'm not super thrilled about having to potentially check every data object defined by a slice to determine whether we match the active flag. This could be bypassed by only checking the flag on the Component (rather than data object), which is just handled by component_objects, but we really need to care about the data object when choosing BlockData to descend into. But it's probably better to be correct than fast. And of course all of this is bypassed if active=None.

A script summarizing some of the behavior:

import pyomo.environ as pyo 
from pyomo.dae.flatten import flatten_components_along_sets
from pyomo.core.base.set import UnindexedComponent_set

m = pyo.ConcreteModel()
m.time = pyo.Set(initialize=[0, 1, 2, 3]) 
m.b = pyo.Block(m.time)
for t in m.time:
    m.b[t].v = pyo.Var()
def c_rule(m, t): 
    return m.b[t].v == 2
m.c = pyo.Constraint(m.time, rule=c_rule)

def display(sets_list, comps_list):
    for sets, comps in zip(sets_list, comps_list):
        print("  %s" % [s.name for s in sets])
        if len(sets) == 1 and sets[0] is UnindexedComponent_set:
            for comp in comps:
                print("  %s" % pyo.ComponentUID(comp))
        else:
            for comp in comps:
                print("  %s" % pyo.ComponentUID(comp.referent))

sets = (m.time,)
print("active=True")
display(*flatten_components_along_sets(m, sets, pyo.Var, active=True))
print()

print("active=False")
display(*flatten_components_along_sets(m, sets, pyo.Var, active=False))
print()

m.deactivate()
m.b.deactivate()
print("m, m.b deactivated; active=False")
display(*flatten_components_along_sets(m, sets, pyo.Var, active=False))
print()
m.activate()
m.b.activate()

m.b[0].deactivate()
print("m.b[:] partially deactivated; active=True")
display(*flatten_components_along_sets(m, sets, pyo.Var, active=True))
print()

m.b[:].deactivate()
print("m.b[:] fully deactivated; active=True")
display(*flatten_components_along_sets(m, sets, pyo.Var, active=True))
print()
m.b[:].activate()

m.deactivate()
m.c.deactivate()
print("m, m.c deactivated; active=False")
display(*flatten_components_along_sets(m, sets, pyo.Constraint, active=False))
print()

m.c[:].activate()
print("m.c[:] activated, active=False")
display(*flatten_components_along_sets(m, sets, pyo.Constraint, active=False))
print()

Outputs:

active=True
  ['time']
  b[*].v

active=False

m, m.b deactivated; active=False
  ['time']
  b[*].v

m.b[:] partially deactivated; active=True
  ['time']
  b[*].v

m.b[:] fully deactivated; active=True

m, m.c deactivated; active=False
  ['time']
  c[*]

m.c[:] activated, active=False

@Robbybp Robbybp changed the title [WIP] Add active filter to flattener Add active filter to flattener Dec 7, 2022
@Robbybp Robbybp marked this pull request as ready for review December 7, 2022 06:15
pyomo/dae/flatten.py Outdated Show resolved Hide resolved
Comment on lines +306 to +312
# If active=False and ctype is not an ActiveComponent (e.g. it is Var)
# we will not generate any components. To prevent this, only pass the
# active argument if we are looking for active components.
c_active = active if check_active else None

# Looks for components indexed by specified sets immediately in our block.
for c in b.component_objects(ctype, descend_into=False, active=c_active):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsiirola This potentially gets slightly simpler if one of the active=False fixes we talked about gets merged. If we remove the active=True default for variables, then we can just pass component_objects the caller's active argument rather than our computed c_active. However, we still need to check if we are dealing with an ActiveComponent so we know whether to check the slice for an active data object below.

Comment on lines +330 to +340
if (
# Yield if (a) we're not checking activity
not check_active
# or (b) we have not sliced and data object activity matches
or (not sliced_sets and new_slice.active == c_active)
# or (c) we did slice and *any* data object activity matches
or (sliced_sets and any(
data.active == c_active for data in new_slice.duplicate()
))
):
yield sliced_sets, new_slice
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we check a slice for an active data object. This is for consistency with behavior when slicing blocks in the hierarchy.

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.

This looks great - I found one tiny little typo.

pyomo/dae/flatten.py Outdated Show resolved Hide resolved
Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

This looks good. Just a couple typos to resolve.

doc/OnlineDocs/advanced_topics/flattener/index.rst Outdated Show resolved Hide resolved
doc/OnlineDocs/advanced_topics/flattener/index.rst Outdated Show resolved Hide resolved
pyomo/dae/flatten.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 87.05% // Head: 86.39% // Decreases project coverage by -0.67% ⚠️

Coverage data is based on head (dbfd792) compared to base (e49b973).
Patch coverage: 93.10% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2643      +/-   ##
==========================================
- Coverage   87.05%   86.39%   -0.67%     
==========================================
  Files         771      771              
  Lines       86301    89380    +3079     
==========================================
+ Hits        75131    77216    +2085     
- Misses      11170    12164     +994     
Flag Coverage Δ
linux 84.16% <93.10%> (+<0.01%) ⬆️
osx 73.63% <93.10%> (-0.01%) ⬇️
other 84.34% <93.10%> (-0.01%) ⬇️
win 81.52% <93.10%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
pyomo/dae/flatten.py 97.48% <93.10%> (-1.11%) ⬇️
pyomo/solvers/plugins/solvers/cplex_direct.py 59.43% <0.00%> (-19.57%) ⬇️
pyomo/solvers/plugins/solvers/mosek_persistent.py 72.78% <0.00%> (-17.51%) ⬇️
pyomo/solvers/plugins/solvers/mosek_direct.py 53.69% <0.00%> (-14.19%) ⬇️
pyomo/solvers/plugins/solvers/CPLEX.py 73.86% <0.00%> (-11.49%) ⬇️
pyomo/util/subsystems.py 89.13% <0.00%> (-8.70%) ⬇️
...ontrib/appsi/utils/collect_vars_and_named_exprs.py 93.75% <0.00%> (-6.25%) ⬇️
pyomo/solvers/plugins/solvers/cplex_persistent.py 46.57% <0.00%> (-6.16%) ⬇️
pyomo/solvers/plugins/solvers/CONOPT.py 40.90% <0.00%> (-4.88%) ⬇️
pyomo/solvers/plugins/solvers/GAMS.py 63.75% <0.00%> (-4.15%) ⬇️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jsiirola jsiirola merged commit 0f59de9 into Pyomo:main Feb 15, 2023
@Robbybp Robbybp deleted the flatten-active-blocks branch March 2, 2023 02:44
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.

Pyomo DAE flatten ignores whether blocks are active
6 participants