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

Feature/measures #392

Open
wants to merge 79 commits into
base: main
Choose a base branch
from
Open

Feature/measures #392

wants to merge 79 commits into from

Conversation

szvsw
Copy link
Collaborator

@szvsw szvsw commented Aug 27, 2022

The old pr was from my fork, re-opening here so that it is in the main repo.

Refactored the measures lib so that

  1. It is broken into smaller classes (Mesasure now makes use of MeasureProperties, and MeasureProperties make use of MeasureActions).
  2. It can support proportional changes (or any other complex value transformer) - e.g. percentage increases
  3. It "disentangles" the targeted objects by default - i.e. it duplicates and replaces the tree of any object which will be mutated. This functionality can be turned off with the disentangle arg, but it is on by default. This means that even if a single template uses the same ZoneLoad for both perimeter and core, you can upgrade just the Perimeter ZoneLoad if desired! And it of course works across objects shared between templates as well. test_disentagle demonstrates this.
  4. kwargs can be passed down to dynamic functions (e.g. lookups, validators, and transformers) from the mutation call, so you can e.g. write a measure which mutates a Zone's LPD, and then decide which zone to mutate at runtime by passing a zone kwarg into your mutation call. This is shown in test_callback_signatures. These dynamic callbacks are signature-checked when they are assigned to an object.
  5. "Presets" can live as classmethods which just spit out a new class object with different instance attribute settings, rather than requiring class inheritance.
  6. Class inheritance presets still work, and additionally you can inherit multiple classes (e.g. SetCOP and SetFacadeThermalResistance) to create a combined measure! pretty cool

I also added a bunch more tests to show the ins and outs of using the MeasureAction/MeasureProp/Measure API.

Point 3 resolves the major issue I had with the previous version in terms of it being ready to publish.

There are a few small outstanding issues or things I wanted to confirm with you:

  • The old facade upgrade was not making use of OpaqueConstruction.r_value - it was instead inferring the actual insulation layer and setting its r_value directly using MaterialLayer.r_value to change the thickness of the insulation layer, as opposed to specifying the whole wall assembly's r_value and then calculating the r_value based off of the the whole thing. I added a new class which does the facade upgrade with a single r_value for the whole assembly, and also kept the original style. I wasn't sure what to use for R-Values for the presets in the version that sets the entire wall assembly's r-value, so some input there would be appreciated.
  • I changed some of the class names for clarity in my opinion, mainly so that they don't reference energy star since the class functionality is independent of the actual preset values - but we can change it back just let me kow
  • I changed some of the arg names to reflect that they are now interacting with setters (i.e. using UpperCamelCase instead of snake_case, e.g. VentilationACH instead of ventilation_ach).
  • It might be nice to write some functions for adding measures together, e.g. an __add__ method or maybe an extend. multi-class inheritance works, see test_class_inheritance
  • I got rid of the changelog functionality since the disentangle functionality means its no longer really necessary. It would be easy enough to add it back in if you think it's worth it, since the new api is very easy to use.
  • I was undecided on creating MeasurePreset class. I think changing the parameters is so straightforward and creating "presets" as classmethods is simple enough that it is not really necessary. with class inheritance working i think this is 100% unnecessary
  • I need to finish up the doc strings with proper google formatting

All checks passing though, including the expanded test_measure.py, so it would be good to review this and see if its ready for merging!

szvsw and others added 30 commits July 2, 2022 11:56
It is simply better to specify the actual children if these umi components instead of relying on their type in DFS traverse algorit
numeric values should be called with Enum(<int>) and strings should be called with Enum[<string>]
@samuelduchesne
Copy link
Owner

My bad, this is the one

@samuelduchesne samuelduchesne mentioned this pull request Oct 19, 2022
@szvsw
Copy link
Collaborator Author

szvsw commented Oct 20, 2022

When we did this in Lisbon, we encountered some issues with the insulation layer upgrade - specifically, they were trying to upgrade single layer brick walls, which resulting in "insulation layer" (the brick) blowing up to like, 10 feet+.

The solution was to create a new measure which automatically adds an insulation layer to the building by creating a new component. The cool thing is, the measures lib can handle those kinds of operations! There's another similar one in there now for changing a single pane window to a double pane window from the default library. Haven't written tests for them yet tho

samuelduchesne and others added 4 commits October 26, 2022 11:47
* The ExpandObjects threaded executor now works even if the epw attribute is not set on an IDF model (#415)

* enables use of expandobjects even if epw is not set

* pint~=0.19

* pint==0.19

* Updates eppy and geomeppy (#418)

* Updates eppy and geomeppy

* Added typing-extensions

* Updated the CI to use actions/setup-python@v4 pip caching (#423)

* Update python-package.yml

* Update setup.cfg

* Added reporting_frequency parameter to IDF class (#422)

* Added reporting_frequency parameter to IDF class

* Implements frequency in Outputs

* Fix flake8

* Fix tests

* Quick fix for sklearn dependency

* Fixes an issue where passing a new attributes to IDF.simulate would fail to create a new cache (#436)

* Fixes an issue where passing a new attributes to IDF.simulate would fail to create a new cache

* reformat

* New function to clear the cache (#437)

* Fixes an issue where passing a new attributes to IDF.simulate would fail to create a new cache

* reformat

* Added clear_cache method

- deletes the settings.cache_folder (files and sub-directories)
- Import from archetypal directly

* Settings in the archetypal.settings module can now be set using environment variables (#438)

* Fixes an issue where passing a new attributes to IDF.simulate would fail to create a new cache

* reformat

* Added clear_cache method

- deletes the settings.cache_folder (files and sub-directories)
- Import from archetypal directly

* Refactored the settings module as a pydantic BaseSettings class

Enables easily setting environment variables

* docs beginning

* format

* Fixes

* rename use_cache to cache_responses

* Updated docs

* Issue with FullService Restaurant file

* fix import

* Update requirements.txt (#441)

* Fixes an issue preventing the EnergyPlus installation folder to be discovered when `as_version` is None (#446)

* Fixes an issue preventing the EnergyPlus installation it be discovered when `as_version` is None

* add missing import

* expand_objects path fix for docker implementation. (#442)

* expand_objects path fix for docker implementation.

* Fixes an issue preventing the EnergyPlus installation it be discovered when `as_version` is None

* add missing import

* Removed conflicting geomeppy requirement.

---------

Co-authored-by: Samuel Letellier-Duchesne <[email protected]>
Co-authored-by: Samuel Letellier-Duchesne <[email protected]>

---------

Co-authored-by: Samuel Letellier-Duchesne <[email protected]>
Co-authored-by: Samuel Letellier-Duchesne <[email protected]>
Co-authored-by: Samuel Letellier-Duchesne <[email protected]>
Co-authored-by: zlehong <[email protected]>
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