Adds the ability to register new functions for EpBunch objects #250

samuelduchesne · 2019-07-25T17:49:30Z

Gentlemen, I have been working a lot with eppy lately and sometimes I needed to develop new functions for convenience. For instance, I needed to create a function to return the total conditioned area of the whole building.

My first reflex was to subclass the IDF class and to add a conditioned_area() as a property (with the @property decorator). See here:

This worked well for an IDF level property. But then, I also needed a similar function for zone EpBunch objects (EpBunch.key == 'ZONE') since I wanted to know the conditioned area of individual zones. Since I didn't want to subclass the EpBunch class as well, I figured I could implement a new type of function decorator instead. This is inspired by how Pandas asks developers to extend pandas functionalities.

My suggestion is this: users should decorate custom functions with the register_epbunch_function decorator. Here is an example:

In this example, the function has also to @property decorator because I what the function to behave like a property (no need for the empty parentheses). Furthermore, in the first decorator, the keys attribute lets you specify which type of EP object should implement the function. For example, I only want my conditioned_area method to work for ZONE objects.

To implement the new functionality, I had to change a few things in the way EpBunch class instances are populated with the default methods. I will try to comment directly in this pull request to guide you guys through my logic.

eppy/bunch_subclass.py

eppy/tests/test_bunch_subclass.py

eppy/tests/test_function_helpers.py

samuelduchesne

I use PyCharm as an IDE, therefore a lot of the changes are simply code cleanups.

santoshphilip · 2019-07-25T18:09:14Z

very very nice. I like this.
I never liked the way eppy implemented the functions like area, volume etc.

I have not looked into your code carefully yet - my mindspace is taken up another project at the moment.

If this works and passes all tests (including ones that you may write), I'll be happy to merge it into the eppy.

We should also develop user documentation for this.

samuelduchesne · 2019-07-25T18:31:48Z

I'm glad you like it. Before merging anything, I suggest we continue on this pull request to improve the logic and to, like you said, create documentation.

Finally, I don't see why we should not implement the decorator logic to the IDF class as well! This would allow users to add functionality without using subclasses.

jamiebull1

Looks good to me in general. I've not thought much about how (if at all) it will affect other tools that build on eppy - but I'll have a good look at geomeppy when I get another chunk of time.

I actually prefer this approach to the patching I've used in geomeppy, so thanks for this contribution. Just want to be sure that we don't introduce anything that might break other's code without a very good reason.

eppy/tests/test_function_helpers.py

eppy/bunch_subclass.py

jamiebull1 · 2019-07-25T18:44:33Z

I don't see why we should not implement the decorator logic to the IDF class as well!

Agree 100%

santoshphilip · 2019-07-25T19:01:46Z

@jamiebull1 , No objections to black.

Also, You make a good point about not breaking other tools built on eppy.
the public repositories that use eppy are here:
https://github.com/santoshphilip/eppy/network/dependents

There may be internal tools that are not public, that we don't know about.

samuelduchesne · 2019-07-25T19:05:55Z

Great!

In the mean time, I don't know if you guys can help me with a small issue with the current logic: Since functions are registered before class instantiation and that the function name is added as an attribute (see this Line) of EpBunch, all functions are "available" on an instance of the class, even though not all of them will appear in __dir__. This can possibly create a weird thing such as being able to call .area on the VERSION epbunch (or any other object), which will throw an error of course. That being said, if a user calls dir(epbunch), the area won't show up since it is filtered out by the functions property.

Does that make sense to you guys?

jamiebull1 · 2019-07-25T19:08:02Z

@samuelduchesne IDF.idd_index["ref2names"]["Surface Names"] should get you the surface names

samuelduchesne · 2019-07-25T19:10:49Z

@jamiebull1 but what about possible surface types? eg.: "BuildingSurface:Detailed", "Surface:Wall", etc...

jamiebull1 · 2019-07-25T19:17:08Z

From memory it's names in the IDD, so surface types. I'm away from a computer right now so can't check.

samuelduchesne · 2019-07-25T19:26:49Z

@jamiebull1 I can confirm it is idf.idd_index["ref2names"]['SurfaceNames'] (no space 😉)

eppy/function_helpers.py

samuelduchesne · 2019-07-25T20:22:20Z

Let's keep a todo list (@jamiebull1 & @santoshphilip, feel free to add to this list)

Todo

Add decorator logic to IDF class.
Create a decorator example which adds a function that has attributes (not just properties).
Document the new logic.

samuelduchesne

Oups, looks like CIs are not happy with that one on some platforms. Ideas why?

santoshphilip · 2019-08-03T22:50:46Z

ouch ! I merged the pull request #251
That merge is creating merge conflict in this pull request.

@samuelduchesne the line number for the error is visible on https://travis-ci.org/santoshphilip/eppy/jobs/563726632

If you look at https://travis-ci.org/santoshphilip/eppy/builds/563726631?utm_source=github_status&utm_medium=notification you will see that three of them failed and one passes. Click on the links to follow up and see why they are failing. If you are not able to figure it out, I'll take a look

samuelduchesne · 2019-08-04T04:06:54Z

ouch ! I merged the pull request #251
That merge is creating merge conflict in this pull request.

@samuelduchesne the line number for the error is visible on https://travis-ci.org/santoshphilip/eppy/jobs/563726632

If you look at https://travis-ci.org/santoshphilip/eppy/builds/563726631?utm_source=github_status&utm_medium=notification you will see that three of them failed and one passes. Click on the links to follow up and see why they are failing. If you are not able to figure it out, I'll take a look

Ok, I'll have a look this week.

jamiebull1 · 2019-08-05T07:27:03Z

The problem is a circular import. You're importing bunch_subclass in function_helpers, but you're also importing register_epbunch_function from function_helpers in bunch_subclass.

Two ways to fix it:

Import function_helpers inside the bunch_subclass.addfunctions function
Move addfunctions into function_helpers

I prefer option 2, as it keeps all the imports at the top of the module. Late imports are almost always a code smell.

codecov-io · 2019-08-05T18:49:55Z

Codecov Report

Merging #250 into develop will decrease coverage by 0.21%.
The diff coverage is 52.38%.

@@            Coverage Diff             @@
##           develop    #250      +/-   ##
==========================================
- Coverage    43.32%   43.1%   -0.22%     
==========================================
  Files           89      90       +1     
  Lines         7568    7611      +43     
==========================================
+ Hits          3279    3281       +2     
- Misses        4289    4330      +41

Impacted Files	Coverage Δ
eppy/tests/test_function_helpers.py	`0% <0%> (ø)`
eppy/tests/conftest.py	`0% <0%> (ø)`	⬆️
eppy/tests/test_bunch_subclass.py	`0% <0%> (ø)`	⬆️
eppy/modeleditor.py	`95% <100%> (ø)`	⬆️
eppy/function_helpers.py	`68.75% <65.57%> (-9.22%)`	⬇️
eppy/bunch_subclass.py	`96.78% <89.28%> (+0.17%)`	⬆️
eppy/tests/test_runner.py	`0% <0%> (ø)`	⬆️
eppy/runner/run_functions.py	`88.8% <0%> (+0.31%)`	⬆️

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 deca838...751a720. Read the comment docs.

jamiebull1

Looking good. Good test coverage. I've suggested a few minor changes, but mainly style things. There's also one issue with a test that I think is not testing what it should be (testing for area vs new_area). I may have misunderstood the intent though.

jamiebull1 · 2019-08-06T09:12:17Z

eppy/function_helpers.py

+    snames = [sname.upper() for sname in snames]
+    func_dict = {
+        "area": (area, snames),
+        "height": (height, snames),  # not working correctly


Are these comments from before the changes?

They are not mine

jamiebull1 · 2019-08-06T09:12:59Z

eppy/function_helpers.py

+    # then it is likely to be a surface.
+    # of course we need to recode for surfaces that do not have coordinates :-(
+    # or we can filter those out since they do not have
+    # the field "Number_of_Vertices"


Filtering is fine, I think.

This is for @santoshphilip I think. Not my code.

eppy/tests/test_bunch_subclass.py

eppy/tests/test_function_helpers.py

jamiebull1 · 2019-08-06T09:28:22Z

eppy/tests/test_function_helpers.py

+            IDF.setiddname(iddfhandle)
+        yield IDF
+
+    @pytest.fixture()


I'd prefer these two fixtures to be in conftest.py. In fact I think there may be equivalent ones there already.

corrected in 3080f0d

eppy/bunch_subclass.py

eppy/tests/test_function_helpers.py

…ing the original test_bunch_subclass idftxt

I think this is related to the fixture scope.

samuelduchesne · 2019-08-08T02:22:52Z

I don't understand why Travis is not happy. I think it's because the modeleditor.IDF's idd has already been set. I still don't understand why we can't change the IDD for another instance of modeleditor.IDF

jamiebull1 · 2019-08-12T07:40:34Z

@samuelduchesne

When I suggested reusing an existing fixture, I meant to use one to give you the base IDF. You can then just add the objects you need in your test. For example:

@pytest.fixture()
def building(base_idf):
    """A building fixture created from base_idf and which adds a Zone, and a BuildingSurface:Detailed
    object"""
    base_idf.newidfobject("Zone", Name="West Zone")
    base_idf.newidfobject(
        "BuildingSurface:Detailed", Name="West Floor", Zone_Name="West Zone", Surface_Type="Floor",
        Vertex_1_Xcoordinate=0,
        Vertex_1_Ycoordinate=0,
        Vertex_1_Zcoordinate=0,
        Vertex_2_Xcoordinate=0,
        Vertex_2_Ycoordinate=10,
        Vertex_2_Zcoordinate=0,
        Vertex_3_Xcoordinate=10,
        Vertex_3_Ycoordinate=10,
        Vertex_3_Zcoordinate=0,
        Vertex_4_Xcoordinate=10,
        Vertex_4_Ycoordinate=0,
        Vertex_4_Zcoordinate=0,
    )
    yield base_idf

This gives you a zone with just a floor surface. You could add a wall and a ceiling to ensure they're filtered out, but that's more testing the conditioned_area algorithm than testing registering functions, so belongs in your application's tests, not eppy's.

As an aside, I'd suggest using almost_equal for the test for surface.tilt == 180.0 since floating point errors often accumulate in calculations on IDF geometry. Alternatively, you could just check if the Surface_Type is Floor, if you know that it will have been set correctly in your IDF.

samuelduchesne · 2019-08-12T13:56:52Z

@samuelduchesne

When I suggested reusing an existing fixture, I meant to use one to give you the base IDF. You can then just add the objects you need in your test. For example:
@pytest.fixture()
def building(base_idf):
    """A building fixture created from base_idf and which adds a Zone, and a BuildingSurface:Detailed
    object"""
    base_idf.newidfobject("Zone", Name="West Zone")
    base_idf.newidfobject(
        "BuildingSurface:Detailed", Name="West Floor", Zone_Name="West Zone", Surface_Type="Floor",
        Vertex_1_Xcoordinate=0,
        Vertex_1_Ycoordinate=0,
        Vertex_1_Zcoordinate=0,
        Vertex_2_Xcoordinate=0,
        Vertex_2_Ycoordinate=10,
        Vertex_2_Zcoordinate=0,
        Vertex_3_Xcoordinate=10,
        Vertex_3_Ycoordinate=10,
        Vertex_3_Zcoordinate=0,
        Vertex_4_Xcoordinate=10,
        Vertex_4_Ycoordinate=0,
        Vertex_4_Zcoordinate=0,
    )
    yield base_idf
This gives you a zone with just a floor surface. You could add a wall and a ceiling to ensure they're filtered out, but that's more testing the conditioned_area algorithm than testing registering functions, so belongs in your application's tests, not eppy's.

As an aside, I'd suggest using almost_equal for the test for surface.tilt == 180.0 since floating point errors often accumulate in calculations on IDF geometry. Alternatively, you could just check if the Surface_Type is Floor, if you know that it will have been set correctly in your IDF.

Totally makes sense. Adjusted in f37e882

jamiebull1 · 2019-08-12T14:20:23Z

Looks good to me. Anything to add, @santoshphilip ?

…ality

samuelduchesne · 2019-08-12T14:50:54Z

Looks good to me. Anything to add, @santoshphilip ?

Currently working on these:

Todo

Add decorator logic to IDF class.
Create a decorator example which adds a function that has attributes (not just properties).
Document the new logic.

samuelduchesne · 2019-08-12T15:07:57Z

@jamiebull1,
On the latest commit (320dbaa), somehow I can't get the tests to work independently. If I run the TestRegisterFunction class everything works fine, but if I run all tests (eg.: run pystest in tests) functions are not registered. Something to do with fixture scopes? Are you able to make it work?

jamiebull1 · 2019-08-12T15:30:05Z

First thing - from eppy.pytest_helpers import almostequal should replace isclose which I think is only for python >3.5.

Second, I get the same error as you for the IDF helper function registration -works when just running that module, but not if running the full test suite. Not sure what's breaking there, and don't have the time to look into it right now. It shouldn't be the fixture scope as that's function by default. I can only think that there must be something else set at a global level that should be cleared after the yield base_idf statement in the fixture.

Implements function register for EpBunch class

a09bb6a

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/bunch_subclass.py Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/bunch_subclass.py Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/bunch_subclass.py Outdated Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/bunch_subclass.py Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/tests/test_bunch_subclass.py Outdated Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/tests/test_function_helpers.py Outdated Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

samuelduchesne marked this pull request as ready for review July 25, 2019 18:07

jamiebull1 reviewed Jul 25, 2019

View reviewed changes

eppy/tests/test_function_helpers.py Outdated Show resolved Hide resolved

eppy/tests/test_function_helpers.py Outdated Show resolved Hide resolved

eppy/bunch_subclass.py Show resolved Hide resolved

eppy/bunch_subclass.py Outdated Show resolved Hide resolved

Replaces print statements with assert statements

c67050b

samuelduchesne added 2 commits July 25, 2019 16:16

Extends the decorator logic directly in function_helper.py

6576867

Fixes other calls to function helpers through the different modules

1c3a4b1

samuelduchesne commented Jul 25, 2019

View reviewed changes

eppy/function_helpers.py Show resolved Hide resolved

samuelduchesne commented Jul 25, 2019

View reviewed changes

samuelduchesne added 2 commits August 5, 2019 14:32

Merge branch 'develop' into feature/decorators

ed0cdc9

Run black on codebase to standardize formatting

f62484a

Moves addfunctions() to function_helper.py to fix circular import issue

88806b2

jamiebull1 reviewed Aug 6, 2019

View reviewed changes

samuelduchesne added 3 commits August 7, 2019 21:48

No need to cast to int + variable name change

48ae29e

Moves fixtures to conftest.py + adjusts test meaning + reverts modify…

3080f0d

…ing the original test_bunch_subclass idftxt

Trying to fix the CI error

36eccd3

I think this is related to the fixture scope.

Uses existing fixture instead of creating a new building IDF object

f37e882

samuelduchesne added 2 commits August 12, 2019 10:24

Uses math.isclose as suggested by PEP 485 for testing approximate equ…

1b456df

…ality

Implements the function registering logic to :class:IDF + testing

320dbaa

samuelduchesne added 2 commits August 12, 2019 11:33

Uses eppy.pytest_helpers.almostequal instead of math.isclose

330f6ef

Merge branch 'develop' into feature/decorators

751a720

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the ability to register new functions for EpBunch objects #250

Adds the ability to register new functions for EpBunch objects #250

samuelduchesne commented Jul 25, 2019 •

edited

Loading

samuelduchesne left a comment

santoshphilip commented Jul 25, 2019

samuelduchesne commented Jul 25, 2019

jamiebull1 left a comment

jamiebull1 commented Jul 25, 2019

santoshphilip commented Jul 25, 2019

samuelduchesne commented Jul 25, 2019

jamiebull1 commented Jul 25, 2019

samuelduchesne commented Jul 25, 2019

jamiebull1 commented Jul 25, 2019 •

edited

Loading

samuelduchesne commented Jul 25, 2019 •

edited

Loading

samuelduchesne commented Jul 25, 2019

samuelduchesne left a comment

santoshphilip commented Aug 3, 2019

samuelduchesne commented Aug 4, 2019

jamiebull1 commented Aug 5, 2019

codecov-io commented Aug 5, 2019 •

edited

Loading

jamiebull1 left a comment

jamiebull1 Aug 6, 2019

samuelduchesne Aug 8, 2019

jamiebull1 Aug 6, 2019

samuelduchesne Aug 8, 2019

jamiebull1 Aug 6, 2019

samuelduchesne Aug 8, 2019

samuelduchesne commented Aug 8, 2019

jamiebull1 commented Aug 12, 2019 •

edited

Loading

samuelduchesne commented Aug 12, 2019 •

edited

Loading

jamiebull1 commented Aug 12, 2019

samuelduchesne commented Aug 12, 2019

samuelduchesne commented Aug 12, 2019

jamiebull1 commented Aug 12, 2019 •

edited

Loading

Adds the ability to register new functions for EpBunch objects #250

Are you sure you want to change the base?

Adds the ability to register new functions for EpBunch objects #250

Conversation

samuelduchesne commented Jul 25, 2019 • edited Loading

samuelduchesne left a comment

Choose a reason for hiding this comment

santoshphilip commented Jul 25, 2019

samuelduchesne commented Jul 25, 2019

jamiebull1 left a comment

Choose a reason for hiding this comment

jamiebull1 commented Jul 25, 2019

santoshphilip commented Jul 25, 2019

samuelduchesne commented Jul 25, 2019

jamiebull1 commented Jul 25, 2019

samuelduchesne commented Jul 25, 2019

jamiebull1 commented Jul 25, 2019 • edited Loading

samuelduchesne commented Jul 25, 2019 • edited Loading

samuelduchesne commented Jul 25, 2019

Todo

samuelduchesne left a comment

Choose a reason for hiding this comment

santoshphilip commented Aug 3, 2019

samuelduchesne commented Aug 4, 2019

jamiebull1 commented Aug 5, 2019

codecov-io commented Aug 5, 2019 • edited Loading

Codecov Report

jamiebull1 left a comment

Choose a reason for hiding this comment

jamiebull1 Aug 6, 2019

Choose a reason for hiding this comment

samuelduchesne Aug 8, 2019

Choose a reason for hiding this comment

jamiebull1 Aug 6, 2019

Choose a reason for hiding this comment

samuelduchesne Aug 8, 2019

Choose a reason for hiding this comment

jamiebull1 Aug 6, 2019

Choose a reason for hiding this comment

samuelduchesne Aug 8, 2019

Choose a reason for hiding this comment

samuelduchesne commented Aug 8, 2019

jamiebull1 commented Aug 12, 2019 • edited Loading

samuelduchesne commented Aug 12, 2019 • edited Loading

jamiebull1 commented Aug 12, 2019

samuelduchesne commented Aug 12, 2019

Todo

samuelduchesne commented Aug 12, 2019

jamiebull1 commented Aug 12, 2019 • edited Loading

samuelduchesne commented Jul 25, 2019 •

edited

Loading

jamiebull1 commented Jul 25, 2019 •

edited

Loading

samuelduchesne commented Jul 25, 2019 •

edited

Loading

codecov-io commented Aug 5, 2019 •

edited

Loading

jamiebull1 commented Aug 12, 2019 •

edited

Loading

samuelduchesne commented Aug 12, 2019 •

edited

Loading

jamiebull1 commented Aug 12, 2019 •

edited

Loading