Import Schedule:Compact as ScheduleRuleset

[samuelduchesne]

Addresses issue #290

I gave it a try to implement a logic to parse Schedule:Compact components. I tried to follow the honeybee-energy API as much as possible.

[CLAassistant]

All committers have signed the CLA.

[chriswmackey]

Thanks, @samuelduchesne . I will try to review at the end of today or early tomorrow.

[chriswmackey]

I was originally thinking that we should not import Schedule:Compact as a ScheduleRuleset because the most common use-case for Schedule:Compact in honeybee right now is when we want to write an entire ScheduleFixedInterval into the IDF (instead of to a CSV file). This means that, for a case like this, you have a Schedule:Compact with 8760 fields and, by extension, you would have a ScheduleRuleset with 365 different rules with your code here. Granted, I think you wrote it in a way that it would still work for this case but importing the schedule to ScheduleRuleset this way is going to use a lot of memory, take a lot of time, and it really doesn't seem to be the desired pathway to import this type of schedule.

So, if I were trying to solve the issue that you referenced in this PR, I would instead have done the following:

• Implement something similar to the code you have here but do it for Schedule:Week:Compact instead of Schedule:Compact. I know that the Schedule:Week:Comapct can be reliably translated to ScheduleRuleset because it uses ScheduleDay instead of arbitrary "until" times and it has a limit on the number of fields.
• Implement the translation of Schedule:Compact to ScheduleFixedInterval instead of ScheduleRuleset.

With this said, I understand what you are trying to do here and I agree that it is useful for several cases where people are creating Schedule:Compact without using Honeybee. So I think we can merge a contribution like this IF we support it as an option and we don't make it the default behavior. I just know that making it the default like what you have now is going to create problems for the cases where people have translated their ScheduleFixedInterval to Schedule:Compact.

So, if you add a boolean option to the extract_all_from_idf_file method here for import_compact and make it =False so that we don't load all of the huge Schedule:Compact to ruleset schedules by default, then I think we can merge this. Dos this sound ok to you?

[chriswmackey]

Whenever I see duplicated code like this, I know that there should be a function for it instead of copy/pasted code. I recommend making a hidden function on the class where you pass the type of day (eg. weekdays) and the rule list to which the rule would be appended.

Then, you don't even need an if statement and you essentially turn 50 lines of duplicated code into 3 lines.

[chriswmackey]

I realized that I should give a little more explanation of what this hidden method would look like if you wanted to get rid fo the if statement. You could just have a dictionary that maps the word "weekdays" to the name of the property on the ScheduleRule class ("apply_weekdays"). Then you can just use the native python setattr() to set the property to True on the ScheduleRule object instance.

And that will save you from a lot of duplicate code.

[samuelduchesne]

Good point: Is this what you had in mind? 0436193

[chriswmackey]

Yes, that is much cleaner. There's just a typo that I noticed in the code here.

[samuelduchesne]

Good catch!

[chriswmackey]

Thanks for the quick update, @samuelduchesne . If you agree with making the import of Schedule:Comapct an option with a False default, just re-request my review once you implement it. Then, if it all looks good, we can merge.

[samuelduchesne]

Hi @chriswmackey! I was wondering how you manage capitalization mismatch for identifiers in IDF files. For instance, in the ~/EnergyPlus-9-2-0/ExampleFiles/HospitalBaseline.idf file (which contains a bunch of good examples of Schedule:Compact) some schedules refer to the "on/off" type limit and others to the "ON/OFF" type limit (capitalized).

When getting those parsed type limits on this Line, the dictionary indexing fails with a KeyError because it cannot find the 'ON/OFF' type limit since only the 'On/Off' key exists in the dict.

For example:

ScheduleTypeLimits,
    On/Off,                  !- Name
    0,                       !- Lower Limit Value
    1,                       !- Upper Limit Value
    DISCRETE;                !- Numeric Type
Schedule:Compact,
    Floor 1 Cafe_B1518SaladBar_Case:1_DefrostSchedule,  !- Name
    ON/OFF,                  !- Schedule Type Limits Name
    Through: 12/31,          !- Field 1
    For: AllDays,            !- Field 2
    Until: 24:00,0.0;        !- Field 3

Schedule:Compact,
    Hours_of_Operation,      !- Name
    On/Off,                  !- Schedule Type Limits Name
    Through: 12/31,          !- Field 1
    For: AllDays,            !- Field 2
    Until: 24:00,1.00;       !- Field 3

[chriswmackey]

This looks good to me @samuelduchesne . I think we can merge this now.

To answer your question about capitalization, I usually explain it by saying "EnergyPlus isn't case-sensitive but Honeybee is." And Honeybee has to be that way given that it is not just an interface for EnergyPlus but an interface and interoperability hub for a range of engines, some of which are case sensitive. So, while it I know it's not the most ideal solution, we just enforce that all IDFs that are exported/imported to/from Honeybee have to obey case-sensitivity. Thankfully, IDFs built with OpenStudio, the IDFEditor, or exported from LBNL WINDOW usually follow case sensitivity and it's just the case of people manually typing in IDF names where we have to worry about this. I guess that is how a lot of the original sample files were built.

So it is what it is. If it ends up becoming an issue at some point, we can always write a simple pre-processor that capitalizes everything in an IDF file (like what EnergyPlus does under the hood).

[chriswmackey]

It's nice to know that I am not the only one who struggles with this convention. Why in the world EnergyPlus couldn't obey how time works, I do not know. But, from the number of thumbs up on this issue, hopefully future developers may be able to avoid it.

[github-actions]

🎉 This PR is included in version 1.67.17 🎉

The release is available on:

• GitHub release
• v1.67.17

Your semantic-release bot 📦🚀