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

Remove deprecated methods for AirWallMaterial, Node, SizingSystem, ZoneAirMassFlowConservation #4632

Merged
merged 43 commits into from Sep 2, 2022

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Jul 28, 2022

Pull request overview

Which deprecated methods should be removed?

  • AirConditionerVariableRefrigerantFlow
  • AirLoopHVAC
  • AirTerminalSingleDuctVAVReheat
  • AirWallMaterial
    • Remove class completely; implement VT to ConstructionAirBoundary
  • AvailabilityManagerNightCycle
  • BuildingUnit
  • CoilCoolingDXSingleSpeed
  • CoilCoolingDXTwoSpeed
  • CoilCoolingWater
  • CoilCoolingWaterToAirHeatPumpEquationFit
  • CoilHeatingGas
  • CoilHeatingWaterToAirHeatPumpEquationFit
  • ConstructionAirBoundary
  • CurveDoubleExponentialDecay
  • DesignDay
  • DesignSpecificationOutdoorAir
  • EvaporativeCoolerDirectResearchSpecial
  • ExteriorFuelEquipment
  • ExteriorLights
  • FanConstantVolume
  • FanZoneExhaust
  • GenericModelObject
  • HeatPumpWaterToWaterEquationFitCooling
  • HeatPumpWaterToWaterEquationFitHeating
  • LayeredConstruction
  • Node (methods deprecated for 9+ years)
  • ParentObject
  • PlanarSurface
  • PlantLoop
  • PumpVariableSpeed
  • ScheduleFile
  • SiteWaterMainsTemperature
  • SizingSystem (note about removing after openstudio-standards 0.29 is released)
    • Remove minimumSystemAirFlowRatio in favor of centralHeatingMaximumSystemAirFlowRatio
    • Remove setMinimumSystemAirFlowRatio in favor of setCentralHeatingMaximumSystemAirFlowRatio
  • SpaceInfiltrationDesignFlowRate
  • StandardGlazing
  • SubSurface
  • ThermalZone
  • ThermostatSetpointDualSetpoint
  • ZoneAirMassFlowConservation (methods deprecated as of OS 1.9.3 that do nothing); also fix RT
  • ZoneHVACEquipmentList
  • ZoneHVACLowTempRadiantConstFlow
  • ZoneHVACPackagedTerminalAirConditioner
  • ZoneHVACPackagedTerminalHeatPump
  • ZoneHVACTerminalUnitVariableRefrigerantFlow
  • ZoneVentilationDesignFlowRate

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 component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated labels Jul 28, 2022
@joseph-robertson joseph-robertson self-assigned this Jul 28, 2022
@tijcolem
Copy link
Collaborator

I think we should create a wiki page that describes the policy for deprecated methods. Do we support 2 versions? 1 Version? Use OS_DEPRECATE. I think the warning should ouput the last supported version.

@joseph-robertson
Copy link
Collaborator Author

@jmarrec If we remove the AirWallMaterial class, do we need to create a VT from Material:AirWall to Construction:AirBoundary? Would it follow the now obsolete code from FT:

    // replace old style air wall Constructions with ConstructionAirBoundary
    for (LayeredConstruction construction : model.getModelObjects<LayeredConstruction>()) {
      if (construction.isModelPartition() && (construction.numLayers() == 1)) {
        MaterialVector layers = construction.layers();
        OS_ASSERT(layers.size() == 1u);

        // if this is an old style air wall
        if (layers[0].optionalCast<AirWallMaterial>()) {
          ConstructionAirBoundary newConstruction(model);
          newConstruction.setName(construction.nameString() + "_ConstructionAirBoundary");

          for (WorkspaceObject source : construction.sources()) {
            for (unsigned index : source.getSourceIndices(construction.handle())) {
              bool test = source.setPointer(index, newConstruction.handle());
              OS_ASSERT(test);
            }
          }

          LOG(Warn, "Construction '" << construction.nameString() << "' has been converted to ConstructionAirBoundary '"
                                     << newConstruction.nameString() << "'.");
          construction.remove();
        }
      }
    }

@jmarrec
Copy link
Collaborator

jmarrec commented Aug 2, 2022

@jmarrec If we remove the AirWallMaterial class, do we need to create a VT from Material:AirWall to Construction:AirBoundary? Would it follow the now obsolete code from FT:

Yes, absolutely. Scan constructions, find those that have only one layers that is an Material:AirWall, create a new Construction:AirBoundary, delete/don't VT the original construction and layer.


Your build error is because you still have files referencing AirWallMaterial,

/srv/jenkins/openstudio/docker-volumes/ubuntu-2004/src/gbxml/ReverseTranslator.cpp:61:10: fatal error: ../model/AirWallMaterial.hpp: No such file or directory

@jmarrec
Copy link
Collaborator

jmarrec commented Aug 22, 2022

@joseph-robertson build error:

/Users/jenkins/git/OpenStudioIncremental/develop/src/model/FloorspaceReverseTranslator.cpp:70:10: fatal error: 'AirWallMaterial.hpp' file not found

@joseph-robertson joseph-robertson changed the title Remove deprecated methods for AirWallMaterial, CoilCoolingDX, Node, SizingSystem, ZoneAirMassFlowConservation Remove deprecated methods for AirWallMaterial, Node, SizingSystem, ZoneAirMassFlowConservation Aug 22, 2022
Comment on lines 77 to 81
* [#4632](https://github.com/NREL/OpenStudio/pull/4632) - Remove deprecated methods for AirWallMaterial, Node, SizingSystem, ZoneAirMassFlowConservation
* Removes the deprecated `AirWallMaterial` class completely, in favor of `ConstructionAirBoundary`
* Removes functions in `Node` that have been deprecated for 9+ years
* Removes deprecated methods in the `SizingSystem` class
* Removes deprecated methods in the `ZoneAirMassFlowConservation` class
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

src/model/PlanarSurface.cpp Show resolved Hide resolved
src/model/SizingSystem.cpp Show resolved Hide resolved
src/model/ZoneAirMassFlowConservation.cpp Show resolved Hide resolved
@jmarrec
Copy link
Collaborator

jmarrec commented Sep 1, 2022

To be honest I kinda forgot I have done #3626 before. https://github.com/NREL/OpenStudio/blob/9eb0b838c20e6e5cf4d0cb5acd2ba3a26703afa3/developer/ruby/deprecated_methods.csv

Maybe we should take everything on that list into this PR.

@jmarrec
Copy link
Collaborator

jmarrec commented Sep 1, 2022

Lots of failure on the OpenStudio-resources' side. I'm trying to fix it.

originally failures

-n '/(test_availability_managers_rb)|(test_baseline_sys03_rb)|(test_baseline_sys04_rb)|(test_baseline_sys09_rb)|(test_baseline_sys10_rb)|(test_coil_waterheating_desuperheater_rb)|(test_coilsystem_dxhx_rb)|(test_coilsystem_dxhx_desiccant_balancedflow_rb)|(test_evaporative_cooling_rb)|(test_fuelcell_rb)|(test_heatexchanger_airtoair_sensibleandlatent_rb)|(test_heatexchanger_desiccant_balancedflow_rb)|(test_heatpump_hot_water_rb)|(test_hot_water_rb)|(test_humidity_control_rb)|(test_humidity_control_2_rb)|(test_ideal_plant_rb)|(test_lifecyclecostparameters_rb)|(test_moisture_settings_rb)|(test_performanceprecisiontradeoffs_rb)|(test_schedule_file_rb)|(test_schedule_fixed_interval_rb)|(test_schedule_fixed_interval_schedulefile_rb)|(test_schedule_ruleset_2012_LeapYear_rb)|(test_schedule_ruleset_2012_NonLeapYear_rb)|(test_schedule_ruleset_2013_rb)|(test_zone_air_movement_rb)|(test_zone_control_contaminant_controller_rb)|(test_zone_fan_exhaust_rb)|(test_zone_mixing_rb)|(test_zoneventilation_windandstackopenarea_rb)/'

image

jmarrec added a commit to NREL/OpenStudio-resources that referenced this pull request Sep 1, 2022
jmarrec added a commit to NREL/OpenStudio-resources that referenced this pull request Sep 1, 2022
cf NREL/OpenStudio#4632
This was done at 1.13, so we don't need to do conditional (based on version) as we won't run tests so far back in time
@jmarrec
Copy link
Collaborator

jmarrec commented Sep 1, 2022

I fixed the regression tests in NREL/OpenStudio-resources#174

@jmarrec
Copy link
Collaborator

jmarrec commented Sep 1, 2022

@joseph-robertson I'll let this one build. Then I'll do another run of NREL/OpenStudio-resources#174 to make sure everything is alright and then we can merge. Thanks for your work!

@jmarrec jmarrec merged commit e6de5b8 into develop Sep 2, 2022
@jmarrec jmarrec deleted the deprec-methods branch September 2, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated component - Model 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.

Remove functions in Node that have been deprecated for 9+ years Remove the deprecated AirWallMaterial class
4 participants