Skip to content
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

feat: added multiplier to type limits #118

Conversation

TfedUD
Copy link
Collaborator

@TfedUD TfedUD commented Jun 20, 2024

This PR adds an additional type limit to the def schedule_type_limit_to_inp to allow for MULTIPLIER type limits to pass

@TfedUD TfedUD requested a review from chriswmackey June 20, 2024 16:13
Copy link
Member

@chriswmackey chriswmackey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @TfedUD ,

I support making this change but it's not implemented correctly. Honeybee-energy ScheduleTypeLimits are objects that have their own properties. They are different from ScheduleTypeLimits in DOE-2 where it's really just a bit of text that denotes what the type is.

So, instead of:

elif type_limit  == 'Multiplier'

it should be:

elif type_limit.display_name  == 'Multiplier'

See here in the honeybee-energy docs for more guidance:
https://www.ladybug.tools/honeybee-energy/docs/honeybee_energy.schedule.typelimit.html

@TfedUD
Copy link
Collaborator Author

TfedUD commented Jun 21, 2024

Hi @chriswmackey I didn't think type_limit had MULTIPLIER as an option.
see typelimit.py here
Also multiplier doesn't work with type_limit component
image

Would the class ScheduleTypeLimit(object) in typelimit.py also need modification to allow for MULTIPLIER to work?
Currently im using the schedule_day_to_inp having modified the code on my install to match the PR, but I can just as easily have the component take a HB Type Limit instead of a text string
image

Should the changes be your aforementioned, as well as adding multiplier to ScheduleTypeLimit?

@chriswmackey
Copy link
Member

Hey @TfedUD ,

EnergyPlus does not have a unit_type for multiplier since multipliers are not really something that you schedule in E+, though you can kinda do this with the EMS. If you wanted to construct a type limit for something like a multiplier in E+, the type limit would probably have discrete_ set to True, and the unit_type would probably be something like Dimensionless, Control or Mode. But that's why I was thinking that maybe we should not rely on the unit_type to tell us whether the type should be translated to eQuest as Multiplier. Maybe we should just see if the user chose the name Multiplier when they created the type and, if so, we translate it to the eQuest Multiplier type:

image

@TfedUD
Copy link
Collaborator Author

TfedUD commented Jul 8, 2024

Sorry about how late this edit was.

@TfedUD TfedUD requested a review from chriswmackey July 8, 2024 13:35
Copy link
Member

@chriswmackey chriswmackey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chriswmackey chriswmackey merged commit f3f280f into ladybug-tools:main Jul 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants