Extend flattener to allow multiple sets #1768

Robbybp · 2021-01-03T23:15:17Z

Summary/Motivation:

The current pyomo.dae.flatten module has a function flatten_dae_components that partitions indexed components into new (reference) components, indexed only (if at all) by a specified set. This PR extends this functionality by allowing multiple sets to be specified. The returned components may be indexed by any or all of the specified sets. This is useful especially for PDE modeling/analysis, where a unique "variable" is defined as something indexed by e.g. time and space.

Changes proposed in this PR:

Overhaul flattener helper functions to allow the more general functionality
Add a user-facing function flatten_components_along_sets that implements this partition
Rewrite flatten_dae_components as a special case of flatten_components_along_sets

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:

I agree my contributions are submitted under the BSD license.
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.

…multiple

codecov · 2021-01-19T19:57:26Z

Codecov Report

Merging #1768 (7c26253) into master (a05199e) will decrease coverage by 0.17%.
The diff coverage is 97.85%.

@@            Coverage Diff             @@
##           master    #1768      +/-   ##
==========================================
- Coverage   75.14%   74.96%   -0.18%     
==========================================
  Files         638      638              
  Lines       91863    94318    +2455     
==========================================
+ Hits        69027    70706    +1679     
- Misses      22836    23612     +776

Impacted Files	Coverage Δ
pyomo/dae/flatten.py	`97.88% <97.85%> (+7.22%)`	⬆️
pyomo/pysp/phsolverserver.py	`7.83% <0.00%> (-58.99%)`	⬇️
pyomo/opt/parallel/pyro.py	`69.48% <0.00%> (-18.71%)`	⬇️
pyomo/solvers/plugins/solvers/CPLEX.py	`84.60% <0.00%> (-4.19%)`	⬇️
pyomo/core/base/global_set.py	`90.69% <0.00%> (-2.33%)`	⬇️
pyomo/pysp/plugins/wwphextension.py	`55.55% <0.00%> (-1.97%)`	⬇️
pyomo/pysp/ph.py	`68.21% <0.00%> (-1.12%)`	⬇️
pyomo/pysp/phutils.py	`72.31% <0.00%> (-0.57%)`	⬇️
pyomo/pysp/phsolverserverutils.py	`68.95% <0.00%> (-0.20%)`	⬇️
pyomo/core/kernel/block.py	`100.00% <0.00%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a05199e...7c26253. Read the comment docs.

…multiple

blnicho

A few minor comments but otherwise I think this looks fine.

pyomo/dae/flatten.py

pyomo/dae/tests/test_flatten.py

blnicho · 2021-01-26T06:37:27Z

pyomo/dae/tests/test_flatten.py

+        #for sets, comps in zip(sets_list, comps_list):
+        #    if len(sets) == 1 and sets[0] is m.d2:
+        #        ref_data = {
+        #                self._hashRef(Reference(m.v_2n[:,('a',1)])),
+        #                self._hashRef(Reference(m.v_2n[:,('b',2)])),
+        #                }
+        #        # Cannot access the data of ^these references
+        #        for comp in comps:
+        #            self.assertIn(self._hashRef(comp), ref_data)


Can this commented code be removed? At a higher level, is this test complete and doing what you expect? The comments seem to indicate that there may have been some issues debugging it.

This test is basically not doing what is expected. The line

self.assertNotIn(('a', 1), ref1._index._ref)

confirms this. ('a', 1) should be a valid index. This is a problem with compatibility between slices and normalize_index.flatten == False. Basically a one-dimensional slice does not properly slice a higher-dimension set in this case. I'm not sure there is an easy fix. The commented code is what I would have expected to work.

Probably should just open an issue about this then remove this test?

Thanks for the clarification. I think the best thing to do is to open an issue and then add a "TODO" comment in this test that references the issue. I think leaving the commented test for future reference is fine as long as we can follow the context of why it's there.

Added comment and opened issue

…nto flatten-multiple

jsiirola

I think this looks good (up do some questions on naming / documentation).

I do have a side question for you: how challenging do you think it would be it be to add a wrapper object around ReferenceDict to support reordering the indices of a reference? That way you wouldn't have to create separate results for [I, J, I] and [I, I, J]. I suspect we could do that pretty easily (in a separate PR).

pyomo/dae/flatten.py

pyomo/dae/tests/test_flatten.py

Robbybp · 2021-01-26T18:51:52Z

This may be a larger can of worms than I should open right now, but I'm not sure I like the name flatten_components_along_sets. "Flatten" seems to imply that we are going from a "higher dimension" object to a "lower dimension" object by multiplying sets into a single index, like we might do for finite elements and collocation points.

This functionality seems closer to a partition of component data objects into containers that preserve indexing by the specified sets. Any thoughts on the name partition_components_by_sets?

jsiirola · 2021-01-26T18:54:19Z

This may be a larger can of worms than I should open right now, but I'm not sure I like the name flatten_components_along_sets. "Flatten" seems to imply that we are going from a "higher dimension" object to a "lower dimension" object by multiplying sets into a single index, like we might do for finite elements and collocation points.

I think this does move from a higher dimension down to a lower one, in that all the index sets other than the ones you are flattening along disappear.

Robbybp · 2021-01-26T19:02:16Z

I think this does move from a higher dimension down to a lower one, in that all the index sets other than the ones you are flattening along disappear.

I usually think of flattening as going from a single 2d array to a single 1d array. Here we are going from high-dimensional objects to many low-dimensional objects. I suppose it still is flattening but it seems to be closer to some sort of partition. It feels like cheating to say we "flattened" an object when really we broke it up enough that we could call each piece flat.

Robbybp · 2021-01-26T21:20:24Z

After a little more thought, I think we are actually doing three things here. First we split components by eliminating any sets we don't care about, second we partition these components according to what sets they're still indexed by, and third we create references that allow the user to access the new components directly.

The first and second steps can be thought of as partitions, the third as flattening. I think the entire procedure can be thought of as viewing components "from the perspective of" the specified sets. Not sure how much this helps naming. Best I can come up with is something like
view_components_from_sets or
view_components_through_sets.

blnicho · 2021-01-27T19:27:10Z

I suggest we keep the current name for now. This is a fairly advanced and specific tool and I think expanding the documentation to include the purpose and intended use of this functionality will be more helpful to "future-us" than trying to find a more precise name at this point.

Robbybp added 30 commits December 2, 2020 09:45

function to create a slice for a particular set

9954229

function for generating sliced components

ea53e7a

function to get flattened components

3494653

bug fix for higher-dimen, non-product sets

30cbc19

add test with Constraint.Skip

92fdb51

cross product sets in a single function call

543e1b1

Catch errors when attempting to iterate over entire index set

903c05a

comments

c13a87e

comments, mostly

e844ec0

tests for new flattener

349bb77

catch invalid component after call

89e6961

extract first entry of len-1 index for compatibility with flatten==False

f38eaab

test with multi-dim sets when flatten==False

bf47abd

beginning test for flattening along a 2-d set

92bd30f

a few lines for debugging

391855a

Merge branch 'master' of https://github.com/Pyomo/pyomo into flatten-…

afa91de

…multiple

lines for debugging

6b30f95

comments

f1b4686

typo

a1bfa4a

tests that flatten over multi-dimensional sets

2a6ea20

comment about an edge case

986e774

remove commented code

6eef67d

add index_map argument that does nothing yet

1d9fed6

use indices that were specified to descend into blocks

8b7f828

use supplied indices when all sets must be sliced

1ba53d8

tests that use supplied indices

91a45e4

checkout changes from other branch

7564598

remove test for KeyError with blocks

f8722ff

remove comments and comment out unused except KeyError

2cb6b18

remove done todos

7694ba1

jsiirola added 3 commits January 19, 2021 09:01

Merge branch 'master' into flatten-multiple

c7db719

Python 2.7 compatibility fixes

cef6bfe

Merge branch 'master' into flatten-multiple

467e745

jsiirola added the AT: PRE-TEST INSPECTED label Jan 19, 2021

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Jan 19, 2021

Merge branch 'master' of https://github.com/Pyomo/pyomo into flatten-…

c4fb599

…multiple

blnicho reviewed Jan 26, 2021

View reviewed changes

Robbybp added 2 commits January 26, 2021 11:06

Merge branch 'flatten-multiple' of https://github.com/Robbybp/pyomo i…

0f20f27

…nto flatten-multiple

updating docstrings and comments per review

2314548

blnicho added the AT: PRE-TEST INSPECTED label Jan 26, 2021

Robbybp mentioned this pull request Jan 26, 2021

Slicing doesn't work with normalize_index.flatten == False #1800

Closed

reference issue for slicing higher dimen sets with flatten==False

0507a52

blnicho approved these changes Jan 26, 2021

View reviewed changes

jsiirola reviewed Jan 26, 2021

View reviewed changes

Robbybp added 2 commits January 26, 2021 14:13

more descriptive model names

bcd22e6

rename _slice to slice_

7c26253

jsiirola approved these changes Jan 26, 2021

View reviewed changes

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Jan 26, 2021

blnicho added the AT: PRE-TEST INSPECTED label Jan 26, 2021

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Jan 26, 2021

blnicho merged commit 29dc786 into Pyomo:master Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend flattener to allow multiple sets #1768

Extend flattener to allow multiple sets #1768

Robbybp commented Jan 3, 2021

codecov bot commented Jan 19, 2021 •

edited

Loading

blnicho left a comment

blnicho Jan 26, 2021

Robbybp Jan 26, 2021

blnicho Jan 26, 2021

Robbybp Jan 26, 2021

jsiirola left a comment

Robbybp commented Jan 26, 2021

jsiirola commented Jan 26, 2021

Robbybp commented Jan 26, 2021

Robbybp commented Jan 26, 2021

blnicho commented Jan 27, 2021

Extend flattener to allow multiple sets #1768

Extend flattener to allow multiple sets #1768

Conversation

Robbybp commented Jan 3, 2021

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Jan 19, 2021 • edited Loading

Codecov Report

blnicho left a comment

Choose a reason for hiding this comment

blnicho Jan 26, 2021

Choose a reason for hiding this comment

Robbybp Jan 26, 2021

Choose a reason for hiding this comment

blnicho Jan 26, 2021

Choose a reason for hiding this comment

Robbybp Jan 26, 2021

Choose a reason for hiding this comment

jsiirola left a comment

Choose a reason for hiding this comment

Robbybp commented Jan 26, 2021

jsiirola commented Jan 26, 2021

Robbybp commented Jan 26, 2021

Robbybp commented Jan 26, 2021

blnicho commented Jan 27, 2021

codecov bot commented Jan 19, 2021 •

edited

Loading