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

Add Occupant Types Data #97

Merged
merged 11 commits into from
Sep 27, 2024
Merged

Add Occupant Types Data #97

merged 11 commits into from
Sep 27, 2024

Conversation

jiangyilin123
Copy link
Collaborator

Description

Adding the occupant types data into the database aims to capture diverse behavioral patterns and create a representative dataset that enables users to improve the building energy modeling and advance research on dynamic occupant behavior using OpenStudio.

Note that this initial design of the occupant types database focuses specifically on office space types and has not yet been integrated into OpenStudio-Standards yet.

Type of change

Specifically, we add support_occupant_types.py,support_occupant_energy_behavior.py, and support_occupant_energy_behavior.py under database_tables. The JSON files corresponding to these data have also been added. Additionally, we included more schedules in the support_schedules.json, which is used to represent occupant schedules in office buildings. The documents have also been updated to explain the occupant types in the database.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Using the existing unit tests and existing functions, we can generate an occupant type dataset, create openstudio.db, export database data into csv or json format, and generate OpenStudio-Standards Data.

  • Unit test pass locally with my changes
  • openstudio.db created with no error with my changes
  • csv and json dataset can be exported with no error
  • json file correctly generated under local openstudio

Test Configuration:
Python 3.12

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I used previous tests to prove my fix is effective or that my feature works
  • Existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@jiangyilin123
Copy link
Collaborator Author

Hi PNNL team,
We have submitted a pull request to add occupant-type data. We updated the documents (Structure. md) to explain our dataset. Is this appropriate? If not, we can have a separate document for this.

@lymereJ
Copy link
Contributor

lymereJ commented Sep 23, 2024

@jiangyilin123 - Thanks a lot for your submission! Do you need it to be merged before the end of the month? Which version of Black are you using to do the formatting?

@jiangyilin123
Copy link
Collaborator Author

@jiangyilin123 - Thanks a lot for your submission! Do you need it to be merged before the end of the month? Which version of Black are you using to do the formatting?

Hi. The following is my environment:
black, 24.8.0 (compiled: yes)
Python (CPython) 3.12.3

@lymereJ
Copy link
Contributor

lymereJ commented Sep 23, 2024

We're currently using version 22.12.0 of Black. Are you able to try it with that version?

@jiangyilin123
Copy link
Collaborator Author

jiangyilin123 commented Sep 23, 2024

We're currently using version 22.12.0 of Black. Are you able to try it with that version

Hi @lymereJ , it would be ideal to merge this submission by the end of this month as it is our FY24 delivery. If that's not possible, merging it before the next expected official release date is another alternative. Thanks!

Copy link
Collaborator

@weilixu weilixu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Overall this looks real good IMO.
I left three comments. Two of them are clarifications and one needs to be reviewed.
Thank you for your contribution @jiangyilin123

getattr_either("co2_generation", record),
getattr_either("co2_generation_units", record),
getattr_either("annotation", record),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

getattr_either function reads the data in the record and put default if failed. This function is used in conjunction with the validate_record_datatype where the validate_record_datatype enforces the check on NOT NULL data and getattr_either function in here to get the attribute but handles it with default if the data is None.
Just want to clarify the intention of these two functions - if that is the intention, then this implementation is good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was my intention.

# tables with foreign keys
"support_occupant_physical_characteristics",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The support_occupant_physical_characteristics do not have foreign keys. Move this line above the comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiangyilin123 - I just want to follow-up on Weili's comment. I see that the support_occupant_physical_characteristics table does not currently use any foreign key, however, it does include schedules fields which I think could be foreign keys. What's your intent here? If you don't want to add the schedule as foreign keys then Weili's comment should be addressed, otherwise, you need to add them as foreign keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the comment. I will follow Weili's suggestion at this stage. Originally, the support_occupant_physical_characteristics has the following code, which sets name in support_schedules as the foreign keys. However, this setting cannot pass the test because name is not a unique value. Do you have any suggestions to do this right?

CREATE_PHYSICAL_CHAR_TABLE = """
CREATE TABLE IF NOT EXISTS support_occupant_physical_characteristics (
    id INTEGER PRIMARY KEY,
    physical_characteristic_name TEXT NOT NULL,
    schedule_activity_level TEXT NOT NULL,
    schedule_clothing_insulation TEXT NOT NULL,
    schedule_air_velocity TEXT NOT NULL,
    work_efficiency NUMERIC,
    co2_generation NUMERIC,
    co2_generation_units TEXT,
    annotation TEXT,
    FOREIGN KEY(schedule_activity_level) REFERENCES support_schedules(name),
    FOREIGN KEY(schedule_clothing_insulation) REFERENCES support_schedules(name),
    FOREIGN KEY(schedule_air_velocity) REFERENCES support_schedules(name)
);

Copy link
Collaborator

@weilixu weilixu Sep 26, 2024

Choose a reason for hiding this comment

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

Got it. This is because the Schedule Table schema does not set the name to be Unique or primary key.
Since this is AOP deliverable, I consider this rather a minor issue for now and not a blocker for the PR. @jiangyilin123 could you put this to the project issue so we can keep track on it?
@lymereJ the suggestion here is whether we want to enforce name to be UNIQUE or ask the support_occupant_physical_characteristics to use the id from support_schedules as the foreign key?

The first option makes the table easier to read
The second option allows duplicated schedule names.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiangyilin123 - Thanks for the feedback, this makes sense. I agree with @weilixu, we'll address this later.

if record.get(f):
assert isinstance(
record[f], str
), f"{f} requires to be a string, instead got {record[f]}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a Not Null behavior, we may want to set cooling_setpoint_units and heating_setpoint_units to be not Null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Piggy-backing on that comment, I think that it would be nice to have a "unit" table so we could standardize units in different tables.

Copy link
Contributor

@lymereJ lymereJ left a comment

Choose a reason for hiding this comment

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

Looks great! I also left a few comments, some informational and some actionable.

"heating_setpoint": 20.5,
"heating_setpoint_units": "Celsius",
"minimum_dimming_level": 0.5,
"annotation": "Based on previous works of literature"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any reference to a publication?

# tables with foreign keys
"support_occupant_physical_characteristics",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jiangyilin123 - I just want to follow-up on Weili's comment. I see that the support_occupant_physical_characteristics table does not currently use any foreign key, however, it does include schedules fields which I think could be foreign keys. What's your intent here? If you don't want to add the schedule as foreign keys then Weili's comment should be addressed, otherwise, you need to add them as foreign keys.

if record.get(f):
assert isinstance(
record[f], str
), f"{f} requires to be a string, instead got {record[f]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Piggy-backing on that comment, I think that it would be nice to have a "unit" table so we could standardize units in different tables.


#### Occupant Types Data Structure
This database is insipried by a previous work Lawrence Berkeley National Laboratory (LBNL) led [(Sun and Hong 2017)](https://www.sciencedirect.com/science/article/abs/pii/S0378778817302013), which established a framework categorizing occupant types into three primary categories:
Copy link
Contributor

Choose a reason for hiding this comment

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

"led by"? Instead of "led". I think we could also do without the "led".

The schedules used in the database were determined based on the PNNL led *Development of Building-Space-Specific Loads for Performance Rating Methods* project. The schedules were derived from the [SBEM-NCN database](https://www.ncm-pcdb.org.uk/sap/page.jsp?id=7), modifications were applied to make them perceived as more realistic such as reducing the occupancy fraction during the day (as space is very rarely fully occupied), or leave a very small fraction of the lights on at night.

### Occupant Types Dataset
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for providing this documentation. We're hoping to provide a more formal documentation of what is included in the database in the future.

The aim of the database is to provide a resources for all types of simulation software, not just openstudio-standards. I think that this work is aligned with this goal. Could you please remove mentions of OpenStudio and openstudio-standards to make it more general purpose? I know that we still have a lot of reference to these products but we're going to start removing them.

Copy link
Contributor

@lymereJ lymereJ left a comment

Choose a reason for hiding this comment

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

I'll approve it for now so merging isn't blocked. @jiangyilin123 please let me know if you want to address some of the other comments.

@jiangyilin123
Copy link
Collaborator Author

Hi @lymereJ , I am addressing all other comments. Just need a bit more time.

@jiangyilin123
Copy link
Collaborator Author

Hi Weili and Jeremy. Thank you for the comments.
I made changes based on your comments. However, there are still several mentions of OpenStudio or openstudio-standard in Strucutre.md file, which were written by the original author, so I left them untouched. Please let me know if there are any other changes required. If not, it's ready to be merged.

@lymereJ
Copy link
Contributor

lymereJ commented Sep 27, 2024

All good, we can address them later! Thanks!

@lymereJ lymereJ merged commit 9417621 into pnnl:develop Sep 27, 2024
2 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.

3 participants