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

Update gbXML schema to v7.03 #4995

Merged
merged 16 commits into from
Oct 16, 2023
Merged

Update gbXML schema to v7.03 #4995

merged 16 commits into from
Oct 16, 2023

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Oct 11, 2023

Pull request overview

Addresses #4966.

Checklist:

  • Update the schema version in XMLValidator::gbxmlValidator()
  • Update the schema and version in ForwardTranslator::translateModel()
  • Include new set of test gbXML v7.03 files, and update/add RT tests

Pull Request Author

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified

@joseph-robertson joseph-robertson added Enhancement Request component - gbXML Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Oct 11, 2023
@joseph-robertson joseph-robertson added this to the OpenStudio SDK 3.7.0 milestone Oct 11, 2023
@joseph-robertson joseph-robertson self-assigned this Oct 11, 2023
Comment on lines +338 to +340
gbxml/11_Jay_St.xml
gbxml/A00.xml
gbxml/Building_Central_Conceptual_Model.xml
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add a few files (v7.03) from autodesk's test suite.

Comment on lines +185 to +191
gbXMLElement.append_attribute("xsi:schemaLocation") = "http://www.gbxml.org/schema http://gbxml.org/schema/7-03/GreenBuildingXML_Ver7.03.xsd";
gbXMLElement.append_attribute("temperatureUnit") = "C";
gbXMLElement.append_attribute("lengthUnit") = "Meters";
gbXMLElement.append_attribute("areaUnit") = "SquareMeters";
gbXMLElement.append_attribute("volumeUnit") = "CubicMeters";
gbXMLElement.append_attribute("useSIUnitsForResults") = "true";
gbXMLElement.append_attribute("version") = "6.01";
gbXMLElement.append_attribute("version") = "7.03";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forward translate with v7.03 header attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Naively would expect some stuff to change in there.
I do realize we only handle geometry (and thermostat), so perhaps it's expected that nothing changes there.

But from a cursory diff of the v6 and v7 schemas, I think we could perhaps handle the new DesignCoolRH and DesignHeatRH by inspecting a potential ZoneControlHumidistat in the ForwardTranslator::translateThermalZone method. Maybe worth an enhancement request, not sure.

@@ -968,3 +968,75 @@ TEST_F(gbXMLFixture, ReverseTranslator_Absorptance) {
EXPECT_EQ(0.7, _material2->visibleAbsorptance()); // default
}
}

TEST_F(gbXMLFixture, ReverseTranslator_v703gbXMLs) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add a new test that does gbXML -> model -> gbXML. Then check that the latter gbXML is valid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer breaking these in individual tests, so we get clearer failures in the event they do start failing.

Comment on lines +193 to +195
std::make_tuple("gbxml/TwoStoryOffice_Trane.xml", 0, 236), std::make_tuple("gbxml/ZNETH.xml", 0, 204),
std::make_tuple("gbxml/11_Jay_St.xml", 0, 0), std::make_tuple("gbxml/A00.xml", 0, 0),
std::make_tuple("gbxml/Building_Central_Conceptual_Model.xml", 0, 3)),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Test XML validation on the new v7.03 files.

Comment on lines +480 to +481
::openstudio::embedded_files::extractFile(":/xml/resources/GreenBuildingXML_Ver7.03.xsd", openstudio::toString(tmpDir), quiet);
return XMLValidator(tmpDir / "GreenBuildingXML_Ver7.03.xsd");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Validate all gbXML files against the v7.03 schema. This is called prior to RT, and after FT.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had noted on the first PR that Matt opened that maybe we should add an enum #4980 (comment)

Quoting myself:

Maybe we should use an enum (or a string with validation... ) and modifying the gbxmlValidator signature to take en optional gbxmlVersion parameter?

enum class GbxmlVersion { Ver601, Ver703 };
XMLValidator gbxmlValidator(GbxmlVersion gbxmlVersion = GbxmlVersion::Ver601);

Otherwise perhaps we should delete the GreenBuildingXML_Ver6.01.xsd altogether.

@joseph-robertson joseph-robertson marked this pull request as ready for review October 12, 2023 16:49
@jmarrec jmarrec mentioned this pull request Oct 16, 2023
19 tasks
Comment on lines +185 to +191
gbXMLElement.append_attribute("xsi:schemaLocation") = "http://www.gbxml.org/schema http://gbxml.org/schema/7-03/GreenBuildingXML_Ver7.03.xsd";
gbXMLElement.append_attribute("temperatureUnit") = "C";
gbXMLElement.append_attribute("lengthUnit") = "Meters";
gbXMLElement.append_attribute("areaUnit") = "SquareMeters";
gbXMLElement.append_attribute("volumeUnit") = "CubicMeters";
gbXMLElement.append_attribute("useSIUnitsForResults") = "true";
gbXMLElement.append_attribute("version") = "6.01";
gbXMLElement.append_attribute("version") = "7.03";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Naively would expect some stuff to change in there.
I do realize we only handle geometry (and thermostat), so perhaps it's expected that nothing changes there.

But from a cursory diff of the v6 and v7 schemas, I think we could perhaps handle the new DesignCoolRH and DesignHeatRH by inspecting a potential ZoneControlHumidistat in the ForwardTranslator::translateThermalZone method. Maybe worth an enhancement request, not sure.

Comment on lines +480 to +481
::openstudio::embedded_files::extractFile(":/xml/resources/GreenBuildingXML_Ver7.03.xsd", openstudio::toString(tmpDir), quiet);
return XMLValidator(tmpDir / "GreenBuildingXML_Ver7.03.xsd");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had noted on the first PR that Matt opened that maybe we should add an enum #4980 (comment)

Quoting myself:

Maybe we should use an enum (or a string with validation... ) and modifying the gbxmlValidator signature to take en optional gbxmlVersion parameter?

enum class GbxmlVersion { Ver601, Ver703 };
XMLValidator gbxmlValidator(GbxmlVersion gbxmlVersion = GbxmlVersion::Ver601);

Otherwise perhaps we should delete the GreenBuildingXML_Ver6.01.xsd altogether.

@@ -968,3 +968,75 @@ TEST_F(gbXMLFixture, ReverseTranslator_Absorptance) {
EXPECT_EQ(0.7, _material2->visibleAbsorptance()); // default
}
}

TEST_F(gbXMLFixture, ReverseTranslator_v703gbXMLs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer breaking these in individual tests, so we get clearer failures in the event they do start failing.

Comment on lines 1020 to 1041
{
openstudio::path inputPath = resourcesPath() / openstudio::toPath("gbxml/Building_Central_Conceptual_Model.xml");

openstudio::gbxml::ReverseTranslator reverseTranslator;
openstudio::gbxml::ForwardTranslator forwardTranslator;

boost::optional<openstudio::model::Model> model = reverseTranslator.loadModel(inputPath);
ASSERT_TRUE(model);

openstudio::path outputPath = resourcesPath() / openstudio::toPath("gbxml/Building_Central_Conceptual_Model_2.xml");
bool test = forwardTranslator.modelToGbXML(*model, outputPath);
EXPECT_TRUE(test);

auto xmlValidator = XMLValidator::gbxmlValidator();

EXPECT_TRUE(xmlValidator.validate(outputPath));
EXPECT_TRUE(xmlValidator.isValid());
EXPECT_EQ(0, xmlValidator.warnings().size());

auto errors = xmlValidator.errors();
EXPECT_EQ(0, errors.size());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In debug (emphasis mine)

[ RUN      ] gbXMLFixture.ReverseTranslator_v703gbXMLs
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)
- [BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
- openstudio_gbxml_tests: /home/julien/Software/Others/OpenStudio/src/gbxml/Test/../../model/../utilities/core/Assert.hpp:37: void boost::assertion_failed(const char*, const char*, const char*, long int): Assertion `false' failed.
- Aborted (core dumped)

In release I get the BOOST_ASSERT issued too, but it doesn't dump.

Click to expand release
[ RUN      ] gbXMLFixture.ReverseTranslator_v703gbXMLs_Building_Central_Conceptual_Model
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[BOOST_ASSERT] <2> Assertion std::abs(z) < 0.001 failed on line 890 of boost::optional<pugi::xml_node> openstudio::gbxml::ForwardTranslator::translateSurface(const openstudio::model::Surface&, pugi::xml_node&) in file /home/julien/Software/Others/OpenStudio/src/gbxml/ForwardTranslator.cpp.
[       OK ] gbXMLFixture.ReverseTranslator_v703gbXMLs_Building_Central_Conceptual_Model (1127 ms)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting a couple LU checks failed on existing files, but no BOOST_ASSERT.

[ RUN      ] gbXMLFixture.ForwardTranslator_Issue_4375
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)
Check failed in file /home/julien/.conan/data/boost/1.79.0/_/_/package/8468b057bf5b2cf5c1e9194777c025a4812bc62f/include/boost/numeric/ublas/lu.hpp at line 298:
detail::expression_type_check (prod (triangular_adaptor<const_matrix_type, upper> (m), e), cm2)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I've checked, the gbXML file does have non-planar surfaces, though I suppose perhaps our tolerance is too small.

from lxml import etree
from typing import List

def locate_surface(root: etree.Element, surface_name: str):
    ns = {
        'k': root.nsmap[None]
    }
    gbxml_sfs = tree.xpath(f"//k:SpaceBoundary[@surfaceIdRef='{surface_name}']", namespaces=ns)
    if not gbxml_sfs:
        raise ValueError(f"Surface {surface_name} was not found")
    n_found = len(gbxml_sfs)
    if  n_found != 1:
        print(f"Found {n_found} surfaces, expected only one")
    return gbxml_sfs[0]

def parse_surface_coordinates(gbxml_sf: etree.Element) -> List[openstudio.Point3d]:
    points = []
    for pt in gbxml_sf.getchildren()[0].getchildren()[0].getchildren():
        x, y, z = pt.getchildren()
        x = float(x.text)
        y = float(y.text)
        z = float(z.text)
        points.append(openstudio.Point3d(x, y, z))
    print(f"Found {len(points)} points")
    return points


xmlPath = Path('../../resources/gbxml/Building_Central_Conceptual_Model.xml')
assert xmlPath.is_file()

tree = etree.parse(str(xmlPath))
root = tree.getroot()
gbxml_sf = locate_surface(root=root, surface_name='aim11994')
points = parse_surface_coordinates(gbxml_sf=gbxml_sf)

# Compute the plane, check if the points are properly on it
plane = openstudio.Plane(points)
print(plane)
tol = 0.001 # Default tolerance
for i, v in enumerate(points):
    if not plane.pointOnPlane(v, tol):
        projected = plane.project(v)
        diff = v - projected
        dist = diff.length()
        print(f"Vertex {i} is not on the plane: dist = {dist:.4f}")

output:

Found 20 points
[0.173657, 0.984806, 8.06117e-05, -20.271]
Vertex 0 is not on the plane: dist = 0.0014
Vertex 1 is not on the plane: dist = 0.0015
Vertex 2 is not on the plane: dist = 0.0016
Vertex 3 is not on the plane: dist = 0.0025
Vertex 4 is not on the plane: dist = 0.0025
Vertex 5 is not on the plane: dist = 0.0016
Vertex 6 is not on the plane: dist = 0.0017
Vertex 7 is not on the plane: dist = 0.0026
Vertex 8 is not on the plane: dist = 0.0026
Vertex 9 is not on the plane: dist = 0.0017
Vertex 10 is not on the plane: dist = 0.0192
Vertex 11 is not on the plane: dist = 0.0182
Vertex 12 is not on the plane: dist = 0.0027
Vertex 13 is not on the plane: dist = 0.0018
Vertex 14 is not on the plane: dist = 0.0019
Vertex 15 is not on the plane: dist = 0.0028
Vertex 16 is not on the plane: dist = 0.0029
Vertex 17 is not on the plane: dist = 0.0019
Vertex 18 is not on the plane: dist = 0.0020
Vertex 19 is not on the plane: dist = 0.0018

Copy link
Collaborator

Choose a reason for hiding this comment

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

max distance: 0.019205503129435128, so almost 2cm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a complex surface too

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm quite sure I don't want a test that actually throws in Debug mode though, I'm going to disable it.
Maybe we should talk to Autodesk about fixing the geometry there, or we need to adjust the tolerance to be more "real-world" friendly.

@jmarrec
Copy link
Collaborator

jmarrec commented Oct 16, 2023

@joseph-robertson I made a couple of changes (disabling one round trip test mostly) and left a few notes that I think we do want to address, but this is good enough to get into RC1 anyways. Letting CI run on it, then we can merge if all green.

@jmarrec jmarrec merged commit 6d120d4 into develop Oct 16, 2023
6 checks passed
@jmarrec jmarrec deleted the gbxml-7_03 branch October 16, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - gbXML Enhancement Request Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update gbXML to 7.03
3 participants