Handle poorly configured streams

[dangunter]

Fixes #1509

Changes proposed in this PR:

• for missing display_{state,display}_vars, set dict to empty and issue a warning
• for states that cannot be treated as indexed components, issue a warning and return state without the index

Legal Acknowledgement

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

1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

[lbianchi-lbl]

Before looking at the code changes: do you have an idea of how much work it would be to add a test exercising this error handling, i.e. a model with missing and/or invalid data that without this PR would result in an error when creating the stream table?

[andrewlee94]

Have we actually seen cases of this? In order for define_state_vars not to exist then someone must have done something wrong and maybe we should be catching this earlier.

[dangunter]

Yes, in Mayo's model

[dangunter]

However if you can suggest where "earlier" might be, I can try to catch it there

[andrewlee94]

So, I think this might need a bigger design discussion. The issue here is the the stream table methods were written only to consider things that were in "material streams", i.e. those related to state blocks in which case the define_state_vars method must exist.

What Mayo was doing was trying to add additional, non-state, data to the stream table which is not supported at the moment. I think a better solution to this might be to design an interface for adding this additional information rather than trying to shoehorn it into the existing interface. There were a few other catches to what Mayo was trying to do as well due to how state data is indexed versus things at the unit level that need to be resolved as well.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 35.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 76.98%. Comparing base (d931060) to head (7a19de5).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
idaes/core/util/tables.py	35.71%	8 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1510      +/-   ##
==========================================
- Coverage   76.99%   76.98%   -0.02%     
==========================================
  Files         382      382              
  Lines       61992    62002      +10     
  Branches    10146    10146              
==========================================
  Hits        47730    47730              
- Misses      11854    11865      +11     
+ Partials     2408     2407       -1     

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

[dallan-keylogic]

Perhaps refactor to

try:
    if true_state:
        disp_func = sb.define_state_vars
    else:
        disp_func = sb.define_display_vars
except AttributeError:
   disp_func = None
   ....
if disp_func is not None:
    disp_dict = disp_func()
else:
    disp_dict = {}

That way we catch only the attribute error for the function not existing, but not attribute errors internal to the function.

[dallan-keylogic]

Actually it looks like you can just use getattr and provide a default to avoid the try-except block. I'm not sure whether None would work as a default, though.

[lbianchi-lbl]

disp_func = lambda: {} should work for that.

[lbianchi-lbl]

@dangunter is still discussing whether this should be handled in the core (i.e. in this repository), or on the UI side.