Uh oh!
There was an error while loading. Please reload this page.
ModelChainResult.__repr__ - #1236
Conversation
cwhanse
commented
Jun 22, 2023
I'm not unhappy with the print of ModelChainResult here. The code is clumsy, because ModelChainResult doesn't hew to any standards for the type of each attribute (e.g., with 2 Arrays, the value can be None, a single float, or a tuple of something). Here's what the report looks like for a simple ModelChain (system with one Array). I'm "head"ing any Series or DataFrames. Any if you read this far, look at the name of the last Series for |
cwhanse
commented
Jun 22, 2023
@kandersolar your call if this is for v0.10 or later. |
kandersolar
commented
Jun 22, 2023
I think using Code for the above: def_mcr_repr(obj):
ifisinstance(obj, tuple):
return"Tuple ("+", ".join([_mcr_repr(o) foroinobj]) +")"ifisinstance(obj, pd.DataFrame):
return"DataFrame ({} rows x {} columns)".format(*obj.shape)
ifisinstance(obj, pd.Series):
return"Series (length {})".format(len(obj))
# scalar, None, other?returnrepr(obj)
def__repr__(self):
lines= ['ModelChainResult:']
attrs= [
'weather', 'solar_position', 'airmass',
'tracking', 'aoi', 'aoi_modifier', 'total_irrad', 'spectral_modifier',
'effective_irradiance', 'cell_temperature', 'dc', 'dc_ohmic_losses', 'losses', 'ac'
]
forattrinattrs:
lines.append(f" {attr}: "+_mcr_repr(getattr(self, attr)))
return"\n".join(lines) |
I had three outcomes in mind:
So I'll drop the |
cwhanse
commented
Jun 23, 2023
Code I'm using to display the report for different PVSystems |
kandersolar
left a comment
There was a problem hiding this comment.
Probably worth a test? We used to have a ModelChain.__repr__ test, but it looks like I incorrectly removed it in #1181, oops. We should probably bring that back.
Uh oh!
There was an error while loading. Please reload this page.
| desc2 = (f'Number of Arrays: {num_arrays} \n') | ||
| attr = 'times' | ||
| desc3 = ('Times (first 3)\n' + | ||
| f'{_head(_getmcattr(self, attr))}' + |
There was a problem hiding this comment.
| f'{_head(_getmcattr(self, attr))}'+ | |
| f'{self.times[:3]}'+ |
Seems like this should be fine as long as it's safe to assume self.times is a DatetimeIndex
There was a problem hiding this comment.
Won't work if times is a DatetimeStamp. Maybe that can't happen, idk.
Uh oh!
There was an error while loading. Please reload this page.
cwhanse
commented
Jun 23, 2023
I added the For the |
| @pytest.mark.parametrize('strategy, strategy_str', [ | ||
| ('south_at_latitude_tilt', 'south_at_latitude_tilt'), | ||
| (None, 'None')]) # GitHub issue 352 |
There was a problem hiding this comment.
Thanks for putting this test back. I think this decorator is no longer needed and can be deleted.
| 'aoi_modifier', 'total_irrad', 'spectral_modifier', | ||
| 'effective_irradiance', 'cell_temperature', 'diode_params', | ||
| 'dc', 'dc_ohmic_losses', 'losses', 'ac'] | ||
| mc_attrs = dir(self) |
There was a problem hiding this comment.
Using dir sorts the list of attributes alphabetically instead of conceptually. Not necessarily a downside.
There was a problem hiding this comment.
Yeah, once I saw the alphabetized list my thought was "that's easier to use"
kandersolar
commented
Jun 23, 2023
Thanks @cwhanse! |
[ ] Tests addeddocs/sphinx/source/api.rstfor API changes.[ ] Adds description and name entries in the appropriate "what's new" file indocs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).