-
Notifications
You must be signed in to change notification settings - Fork 84
Print properties contained in Seawater prop model #1686
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
base: main
Are you sure you want to change the base?
Conversation
…names, descriptions, and units later
| import pandas as pd | ||
|
|
||
|
|
||
| def print_property_metadata(prop_pkg, return_df=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a simpler interface would be get_property_metadata. Wrapping that in a print statement is very little burden on the caller and then you don't need this keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean make it a method on parameterblock so that I don't need prop_pkg keyword? Or did you mean remove the need for returning df in this function and focus on getting the metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he means rename this get_property_metadata and return the df and then the user can do what they want?
I like the idea of this being a method on ParameterBlock. But could you not just use this function and then @classmethod to avoid adding this code to every property model?
| prop_pkg: The property model ParameterBlock (e.g., m.fs.properties) | ||
| return_df: If True, returns a Pandas DataFrame instead of printing. | ||
| """ | ||
| metadata = prop_pkg.get_metadata() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the method (below). I suppose we can add a check for the _metadata attribute and raise an exception if it doesn't exist. Do you know of a better check to ensure the property model is a property model? Another idea is to check whether the prop_pkg input is a ParameterBlock.
@classmethod
def get_metadata(cls):
"""Get property parameter metadata.
If the metadata is not defined, this will instantiate a new
metadata object and call `define_metadata()` to set it up.
If the metadata is already defined, it will be simply returned.
Returns:
PropertyClassMetadata: The metadata
"""
if cls._metadata is None:
pcm = PropertyClassMetadata()
cls.define_metadata(pcm)
cls._metadata = pcm
# Check that the metadata was actually populated
# Check requires looking at private attributes
# pylint: disable-next=protected-access
if pcm._properties is None or pcm._default_units is None:
raise PropertyPackageError(
"Property package did not populate all expected metadata."
)
return cls._metadata
|
|
||
| if return_df: | ||
| return pd.DataFrame( | ||
| {"Property Description": docs, "Model Attribute": vars, "Units": units} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest column headers with no spaces. "Name", "Description", "Units". The prefix "Property" is implied in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be a dataframe? Why not just a dict?
| {"Property Description": docs, "Model Attribute": vars, "Units": units} | ||
| ) | ||
|
|
||
| # Pretty-print |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so again, I would just return the data frame. People know how to print a dataframe in a basic way in a terminal (i.e. just pass obj to print()) and a notebook will do it for you. If you really must print for the user, I'd suggest a separate method print_property_metadata that consists of basically :
pprint.pprint(get_property_metadata(*args).to_string(index=False))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue with the way it's printing now or any advantage to doing it differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue we should not return a dataframe in the first place.
The reason is we don't want to maintain code specific to pandas for printing stuff.
I think we we do want to display/print information we should use native python display tools otherwise we might run into system quirks ... (I wounder if pyomo table printer would work for this - also can look in my PR for WaterTAP flowsheet block for displaying a table - it uses native pyomo printer... ?)
| assert "kg/s" in df["Units"].values | ||
|
|
||
|
|
||
| def test_print_property_metadata_pretty_print(capsys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just test that there are, say, between 100 and 1000 characters in the output.
| self.set_default_scaling("diffus_phase_comp", 1e9) | ||
| self.set_default_scaling("boiling_point_elevation_phase", 1e0, index="Liq") | ||
|
|
||
| def list_properties(self, return_df=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these seem completely generic, so should be on a base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but I started with this first before attempting to add to a base class (which would probably be best on IDAES repo but would slow things down for us)
|
|
||
| # transforming constraints | ||
| transform_property_constraints(self) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is normal in IDAES but I find this way of creating data very verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too, but given the current IDAES functionality and the way metadata is handled and reported back, this was the cleaner way of producing the list of property names actually supported on this property model, without the noise. Open to suggestions if there's a slicker approach, but this is meant a quick fix, with planned refinement in subsequent PRs (this quarter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is redundant, don't the pyomo vars in the property block contain all of the same information? Would it not be easier to make a list of the "vars" you want to display and simply grab it?
| capsys.readouterr() | ||
| m.fs.props.list_properties() | ||
| captured = capsys.readouterr().out | ||
| expected_output = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tets for string matches are not a good idea imho. at a minimum use a regex to look for some things least likely to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting this sort of response! I was aiming for a quick test to make sure my seawater prop model was listing the correct properties. Can reconsider this, but I was hoping to get something merged asap and revise in subsequent PR(s)
MarcusHolly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intent is just to get something merged in preparation for Friday, then I think this is good as is. None of Dan's comments strike me as urgent, so these (and other) refinements can be made in future PRs.
avdudchenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, I am not 100% sure about relying on pandas data frames for data storage and printing.
My biggest question is can't we pull most of the information here from already build pyomo vars defining the supported properties?
I almost wounder if this just need a registration class that lets you track vars you want to report and provide ways to both interact with?
|
|
||
| if return_df: | ||
| return pd.DataFrame( | ||
| {"Property Description": docs, "Model Attribute": vars, "Units": units} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be a dataframe? Why not just a dict?
| {"Property Description": docs, "Model Attribute": vars, "Units": units} | ||
| ) | ||
|
|
||
| # Pretty-print |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue we should not return a dataframe in the first place.
The reason is we don't want to maintain code specific to pandas for printing stuff.
I think we we do want to display/print information we should use native python display tools otherwise we might run into system quirks ... (I wounder if pyomo table printer would work for this - also can look in my PR for WaterTAP flowsheet block for displaying a table - it uses native pyomo printer... ?)
|
|
||
| # transforming constraints | ||
| transform_property_constraints(self) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is redundant, don't the pyomo vars in the property block contain all of the same information? Would it not be easier to make a list of the "vars" you want to display and simply grab it?
Fixes/Resolves:
NA
Summary/Motivation:
While preparing materials for Academy, realized it isn't very easy to list all properties on a property model.
One would need to dive into source code or technical documentation.
This PR attempts to add
list_propertiesandlist_properties_as_dataframemethods to the SeawaterParameterBlock, using a new utility helper function,print_property_metadata.This also required defining a separate PropertySet for Seawater--otherwise, you'll get back a long list of all supported IDAES properties that aren't necessarily supported in the seawater prop model.
Can extend this to other property models once we cover this PR.
Example implementation:
Output:
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: