-
Notifications
You must be signed in to change notification settings - Fork 24
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
Update to PySAM v5.0 #104
Update to PySAM v5.0 #104
Conversation
raise OCHREException('Must specify PV tilt and azimuth, or provide an envelope_model with a roof.') | ||
roofs = [bd.ext_surface for bd in envelope_model.boundaries if 'Roof' in bd.name] | ||
# Use roof closest to south with preference to west (0-45 degrees) | ||
roof_data = pd.DataFrame([[bd.tilt, az] for bd in roofs for az in bd.azimuths], columns=['Tilt', 'Az']) |
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.
Have we checked this would work for a flat roof (say multifamily)?
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.
It should work for flat roofs, we should test that. It won't work for units
that have another unit above them. It's also not too hard to just specify the
tilt and azimuth in the dwelling_args
.
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 we add some error checking to fail gracefully if there's a unit above? I can put together a couple test files (flat roof, unit above) this afternoon in BEopt if that's helpful here.
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.
Just added an error. I think some BEopt tests would be good.
|
||
# Inverter constraints, if included; if not, assume no limits and 100% efficiency |
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.
Rather than 100% efficiency, should we try to pick something that lines up with the SAM defaults for inverter efficiency? Or is inverter efficiency included in the PV inefficiencies so that would be double counting?
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.
This note is old, we are using SAM's inverter efficiency value, unless the
user specifies something else
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.
Left you a few comments, once we resolve the discussion this is ready to merge. Nice job Michael!
@jmaguire1 can we merge this now? It's ready to go, but we haven't tested with new BEopt cases. I have tested with run_dwelling, and everything seems to be in good shape. |
Addresses #1