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

create new types for taiwan project #1384

Merged
merged 26 commits into from
Dec 3, 2024
Merged

Conversation

terryterpt
Copy link
Contributor

@terryterpt terryterpt commented Jul 26, 2024

Hi, I am terry from Primustech MSI and currently doing the DBO for the new Google project in Taiwan.

I am creating this pull request to add entity and abstract types for this project.

Copy link

google-cla bot commented Jul 26, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ontology/yaml/resources/HVAC/entity_types/ABSTRACT.yaml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
@cstirdivant
Copy link
Collaborator

cstirdivant commented Aug 5, 2024

@terryterpt I am pausing my review until you address a few things across this PR:

  • Address the validator error
  • Remove all references to a specific building code
  • All AHU types must have some ECON type
  • Discourage the use of individual fields on canonical types, please review to ensure you can model as many fields within an abstract type instead
  • Add guids to all new abstract and canonical types

@cstirdivant cstirdivant self-assigned this Aug 16, 2024
@terryterpt
Copy link
Contributor Author

@cstirdivant , I have rectify the following items, please help to review my commit again.

  1. Address the validator error
  2. Remove all references to a specific building code
  3. All AHU types must have some ECON type
  4. Discourage the use of individual fields on canonical types, please review to ensure you can model as many fields within an abstract type instead
  5. Add guids to all new abstract and canonical types

.gitignore Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FAN.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/DOAS.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
@cstirdivant
Copy link
Collaborator

cstirdivant commented Sep 4, 2024

Terry,

The PR name and description still has the building specific name, please remove.

In general, this PR is not acceptable.

  • Please make sure you only have yaml files in your PR. The proper validation checks won't run if you have python files in your PR.
  • The AHU types are messy, refrain from defining individual fields on the canonical types (try to use only abstract types or extend existing ones to meet your needs)
  • AHU types should have some heating/cooling temperature control (you define CHW/HW valve control and temperature control separately, but I am assuming they are actually interconnected -- ie the valve controls to a temperature setpoint).

@terryterpt terryterpt changed the title create new types for TW-NTC-TPKE project create new types for taiwan project Sep 5, 2024
@terryterpt
Copy link
Contributor Author

Hi @cstirdivant ,

I have done the following:

  • I have extended the general type AHU & DOAS with opt-use fields, so there is on longer any individual fields.
  • I have updated the AHU types with the valve controls linked to the temperature setpoint.

@terryterpt
Copy link
Contributor Author

Hi @cstirdivant , please help to review my commit again.

Copy link
Collaborator

@shambergoldstein shambergoldstein left a comment

Choose a reason for hiding this comment

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

Apologies for the delay on this review.

In order to omit the python file changes from the PR you must restore them to their original contents rather than deleting them.

.gitignore Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FAN.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FAN.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/CHWS.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/CH.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/CHWS.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/CH.yaml Outdated Show resolved Hide resolved
@terryterpt
Copy link
Contributor Author

Hi @shambergoldstein,

I have the CH type CH_SS_CC2XM_REFSM2X_REFPM2X with the setpoint that is indeed hidden in the BMS system.
Secondly, the CHWS_RWTM is indeed just a temperature sensor on it own with no setpoint

Copy link
Collaborator

@shambergoldstein shambergoldstein left a comment

Choose a reason for hiding this comment

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

Only yaml files should be in the PR, please remove any others

@terryterpt
Copy link
Contributor Author

@shambergoldstein , I have removed the files that are not Yaml

ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/AHU.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FAN.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/DOAS.yaml Outdated Show resolved Hide resolved
ontology/yaml/resources/HVAC/entity_types/FCU.yaml Outdated Show resolved Hide resolved
@shambergoldstein shambergoldstein merged commit 10a21ed into google:master Dec 3, 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