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 #345

Closed
wants to merge 27 commits into from
Closed

Feature/measures #345

wants to merge 27 commits into from

Conversation

szvsw
Copy link
Collaborator

@szvsw szvsw commented Jul 2, 2022

Expands on the original features commit to implement some notion of a change log while also simplifying the syntax for users to define their own measures, while still allowing for dynamic measures.

Not yet ready to merge, but I have added some notes and discussion points below.

Tried to be detailed so that the vision for it is clear!

Once it's ready to commit, I can fill in measures/README.md

The add_modifier method

The method handles adding named props to the measure as attrs, constructing a lambda which will mutate building templates once provided (stored in the measure as an attr), and constructing lambdas which handle generating changelog previews.

This is the major update to the library. It provides an API for defining custom measures which mutate both static and dynamic paths (i.e. an insulation layer), so it should cover the majority of use cases. It does not yet support using computed values based off input building templates, but it shouldn't be too hard to integrate those. It also potentially makes measures serializable, at least those which do not rely on dynamic pathfinding or computed values, however I have not yet implemented any serializer methods.

The basic idea is that users will:

  1. Declare a new class which inherits Measure as before
  2. In the __init__ method for their class, they call self.add_modifier with the relevant args for an action that the measure executes.
  3. repeat step 2 for as many actions as they would like the measure to include

Args:

  • modifier_name: string - the name of the action. this ends up as the attr name for the action callback (as before, e.g. SetPerimeterCoolingCop) which will ultimately get called when applying the measure.
  • modifier_prop: string - the name of the attr which will store the value to use (e.g. cooling_cop). Users can then later change the value if they wish to mutate the measure's action, e.g. myCOPmeasure.cooling_cop = 2 and the measure will be dynamically updated to handle the new action, since the mutators dynamically reference this attr. Modifier actions which are created with the same modifier_prop value will use a shared prop (e.g. cooling_cop can be used for the action which modifies perimeter and core cooling COPs). These values can also be initialized as class attributes, but they will always be converted to instance vars at runtime (i.e. always accessed by getattr(self,modifier_prop)
  • default: any - the value which will be used to overwrite the targeted path for the modifier action. Typically a number, but it should probably still work passing in an object, though I have not tested yet. That does suggest the ability to define the upgrade as completely replacing one window entire construction with another, for instance.
  • object_address: list<str | int> | (BuildingTemplate): list<str | int> - where to find the parameter to mutate - e.g. [ "Perimeter", "Ventilation"]. It can also accept a function which takes in the building template and constructs the path dynamically (this is done in the thermal insulation upgrade measure). The path can be a mixture of string keys and indices as well (also shown in the insulation upgrade). It might be nice to allow this to be specified as a .` separated string rather in addition to an array, but that can be an easy future feature to add.
  • object_parameter: str | int | (building_template: BuildingTemplate, object_address: any): str | int" - the name or index of the final entity to mutate in the template (e.g. ScheduledVentilationAch`). it can also be a function which dynamically determines the parameter name/index.
  • validator: (original_value: any, new_value: any, root: any): boolean - this is an optional function which can be provided to provide a test which will run before applying the modification action - e.g. to prevent lowering a parameter if desired. the building_template will also be supplied to the root argument if any contextual validation needs to be done. The API for this maybe needs a little work, I think it will break if you don't declare your validator with a root arg, even if it is unused.

Here is an example of how it is used, in the SetCOP measure's __init__ method:

self.add_modifier(
    modifier_name="SetCoreCoolingCop",
    modifier_prop="cooling_cop",
    default=cooling_cop,
    object_address=["Core", "Conditioning"],
    object_parameter="CoolingCoeffOfPerf",
)
self.add_modifier(
    modifier_name="SetPerimeterCoolingCop",
    modifier_prop="cooling_cop",
    object_address=["Perimeter", "Conditioning"],
    object_parameter="CoolingCoeffOfPerf",
)

For now, I have left add_modifier as a plain method, but w could easily imagine bundling it up into a small class.

The next thing to do would be to add support for computed values - e.g. an argument with a name like compute which takes in a function that computes the new value dynamically based off of data in the template. I seemed to recall that you mentioned there was a method somewhere for setting the r-value of a wall to a specific value by dynamically resizing the insulation layer, which is an obvious candidate for testing that functionality. I noticed that the existing insulation upgrade which was already written was just using fixed values as of now.

Changelog methods

https://github.com/szvsw/archetypal/blob/f8d1560c0ae2ec0966f0cd6f5462144a1fee63a0/archetypal/template/measures/measure.py#L74

These methods generate a changelog, or really, a schedule of what will be changed if the measure is applied. The main utility here is being able to preview to the user changes that will be executed - use case for me would be in ubem.io as a person is deciding what measures to apply. Maybe we just need to pick a better name for these methods.

Another thing to consider might be abstracting a change into a small Change class but that is maybe overkill.

changelog should be able to handle dynamic values but it can't quite yet

Args at runtime

Forgot about these while I was working, so I have not integrated these into the modifier actions yet, but it shouldn't be hard. Can you give me an example of a measure where you might imagine passing runtime args?
https://github.com/szvsw/archetypal/blob/f8d1560c0ae2ec0966f0cd6f5462144a1fee63a0/archetypal/template/measures/measure.py#L68

Accessing Deeply Nested Attrs

These are a few module-level helper functions.

https://github.com/szvsw/archetypal/blob/f8d1560c0ae2ec0966f0cd6f5462144a1fee63a0/archetypal/template/measures/measure.py#L10

When specifying upgrades, the user will define a path to a parameter as an array of attr names (or a dynamic path constructor). These functions are used to get/set deeply nested attributes/values within a building template based off of a path array. I wasn't sure if there was something like this in the library already, but it was simple enough to write. It may in fact be worth it to adapt these a little bit and insert them at the umi template level?

Another convenient thing to do might be to allow the user to provide the path in a single string, and then generate the array by splitting it on . symbols.

Future

  • measure arithmetic - it would be pretty easy to imagine an __add__ method for Measures - it would be especially easy if we wrapped up the action_modifiers into a small class, as then the modifiers could be stored as a list in the measure and then it would just be list concatenation. still very simple to do as is.
  • serialization - some measures are serializable (i.e. those which only make use of fixed paths and serializable values to write into those paths). I'm not sure how useful this is, but it could be interesting?

Copy link
Owner

@samuelduchesne samuelduchesne left a comment

Choose a reason for hiding this comment

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

Very intrigued with your use of the reduce function. I need to look more into that :)

@szvsw
Copy link
Collaborator Author

szvsw commented Jul 3, 2022

Yeah the nested accessing would be a little clearer probably if UmiBase implemented __getitem__ / __setitem__ to enable accessing all of the template entity attributes with square bracket syntax.

Another way to have implemented the nesting traversal would have been with a recursive call which also maybe might have been clearer... idk

I'm also not 100% sold on the "address" vs "parameter" distinction- they could also be a single array from the start, and then just grab the last item if you want the "parameter" somewhere or the first n-1 items if you want the "address". Both patterns seems fine.

@samuelduchesne
Copy link
Owner

One thing that would be helpful is not only examples of how to create measures (which will eventually go in the readme) but also a bunch a unit tests that can show the dos and don'ts and capture the line coverage. Coveralls is setup btw (I just realized that it was skipped for the past forever) but it should be working now.

@codecov-commenter
Copy link

Codecov Report

Merging #345 (f43322d) into main (bcac75f) will decrease coverage by 1.12%.
The diff coverage is 70.88%.

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
- Coverage   81.51%   80.39%   -1.13%     
==========================================
  Files          52       56       +4     
  Lines        8726    10547    +1821     
==========================================
+ Hits         7113     8479    +1366     
- Misses       1613     2068     +455     
Impacted Files Coverage Δ
archetypal/idfclass/load_balance.py 0.00% <0.00%> (ø)
archetypal/idfclass/util.py 69.56% <0.00%> (-2.08%) ⬇️
archetypal/template/load.py 88.67% <ø> (+1.00%) ⬆️
archetypal/template/ventilation.py 90.93% <ø> (+0.95%) ⬆️
archetypal/template/zone_construction_set.py 95.78% <ø> (+0.15%) ⬆️
archetypal/idfclass/variables.py 60.00% <28.57%> (-15.41%) ⬇️
archetypal/eplus_interface/basement.py 40.76% <33.33%> (+0.59%) ⬆️
archetypal/cli.py 36.42% <50.00%> (-39.49%) ⬇️
archetypal/idfclass/idf.py 70.46% <57.34%> (-3.68%) ⬇️
archetypal/idfclass/sql.py 60.32% <60.32%> (ø)
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28b18e0...f43322d. Read the comment docs.

@szvsw
Copy link
Collaborator Author

szvsw commented Jul 4, 2022

  • decided to get rid of the address/parameter distinction. I think it's simpler to just have the full path as the address argument.
  • tried to make add_modifier a little cleaner and more readable by turning some of the lambdas into named functions
  • I also tried to make the get_path / set_path workflow a little clearer and added comments

This is my first time using pytest, so if you have any feedback, feel free to let me know. I wrote two tests for add_modifier:

  • test_create_basic_measure tests creating a new measure class which inherits Measure
  • test_validated_measure tests creating a new measure with a validator - for this one, I just used the base Measure class and directly called add_modifier.
  • I haven't written one yet which demonstrates using a callable object address, though the facade measure uses that so it does get tested.
  • I haven't written explicit tests for the changelog functionality yet.

The tests all pass fine when running pytest tests/test_measure.py, but I noticed in the github test that the pytest log does not seem to include test_measure.py. Wasn't sure if there was a config file or something somewhere which needs to register the TestMeasure class?

@samuelduchesne
Copy link
Owner

Yeah the nested accessing would be a little clearer probably if UmiBase implemented __getitem__ / __setitem__ to enable accessing all of the template entity attributes with square bracket syntax.

Another way to have implemented the nesting traversal would have been with a recursive call which also maybe might have been clearer... idk

I'm also not 100% sold on the "address" vs "parameter" distinction- they could also be a single array from the start, and then just grab the last item if you want the "parameter" somewhere or the first n-1 items if you want the "address". Both patterns seems fine.

#349 could help!

@samuelduchesne
Copy link
Owner

@szvsw I see tests for test_measure.py in the GitHub Actions log. Looks fine! Are there any remaining tasks for this PR?

@szvsw
Copy link
Collaborator Author

szvsw commented Jul 28, 2022

Yeah, so one of the things that is, I think, a little bit confusing about this is that when you apply a measure to a template, you are potentially mutating many templates (eg updating a wall construction which is used in multiple templates). This is probably kind of confusing. Also, the way I wrote the automatic changelogger does not flag these either.

I meant to bring this up as a discussion point with you at some point.

  • One option could be duplicating and replacing the affected objects so that only the desired template is mutated.
  • Another option could be using the to_graph method and tracing back targeted objects to a list of templates they belong to and flagging them that way (similar to how I am generating the “ancestors” tree on the node editor)
  • or alternatively, if there is some method to quickly generate a list of parent templates for any deeply nested component
  • Another option would be to have a disentangle method for UmiTemplateLibrary which would essentially be the opposite of unique_components - you would give it a list of classes to skip disentangling (eg materials) and then it would create a new unique component for each component in a non-skipped class per template. This might be a good function to have in UmiTemplateLibrary regardless.

maybe this is a non issue though! I just thought that the verbiage of “apply measure to template” is potentially confusing when it can mutate other templates, figured it should be discussed further.

@samuelduchesne
Copy link
Owner

Reopened at #392

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