Merge idf.get_surfaces() and idf.get_surfaces_with_zone_multiplier()

[nheeren]

Having redundant functions doing the same thing creates extra work (e.g. all changes in one function must be carried over to the other) and error prone (e.g. when one forgets to carry changes over to the other function).

get_surfaces_with_zone_multiplier() was introduced by @anistad to make the RT archetype work.

Probably the two functions do already differ due to ongoing development on the former.

Technically both functions do the same, but the zone multiplier is applied. Probably get_surfaces() should always check if a multiplier is present and multiply either by 1 or by the number of zones.

[CBreton026]

I have some time tomorrow, I'll have a look at it and see if I can merge the functions!

[CBreton026]

I started looking into this. I also noticed there is a similar issue with idf.extract_surfaces() and idf.extract_surfaces_zone_multipliers(), I'll see if I can figure something out at the same time.

[nheeren]

I started looking into this. I also noticed there is a similar issue with idf.extract_surfaces() and idf.extract_surfaces_zone_multipliers(), I'll see if I can figure something out at the same time.

Smart – thank you!

[CBreton026]

I created a new branch (issue_57) to work on this.

I'm making progress on merging the functions. At the same time, I'm trying to refactor the functions into smaller ones. Idf.get_surfaces_with_zone_multiplier() is a bit long, and does several different things.

I ended up deleting idf.extract_surfaces_zone_multipliers(), as I couldn't find anywhere it was used. BuildME ran fine after deleting it, which seems to confirm it was dead code.

[CBreton026]

I'm done! I tested the new get_surfaces() function, and it reproduces the exact same results as the previous two functions for both SFH and RT archetypes. I'll send a commit and pull request !

[CBreton026]

To finalise the commit, I have a few questions regarding the last part of get_surfaces_with_zone_multipliers():

    if res_scenario == 'RES2.1' or res_scenario == 'RES2.1+RES2.2':
        floor_slab_constr = constr_list['Concrete_floor_slab-' + res_scenario].Name
        surfaces['int_floor_second_floor'] = create_surrogate_slab(temp_surface_areas['footprint_area'], floor_slab_constr)
        surfaces['basement'] = create_surrogate_basement(temp_surface_areas['footprint_area'], slab_constr)
        surfaces['slab'] = create_surrogate_slab(temp_surface_areas['footprint_area'], slab_constr)
        surfaces['basement1'] = create_surrogate_basement(temp_surface_areas['footprint_area'], slab_constr)
        surfaces['slab1'] = create_surrogate_slab(temp_surface_areas['footprint_area'], slab_constr)
    else:
        surfaces['basement'] = create_surrogate_basement(temp_surface_areas['footprint_area'], slab_constr)
        surfaces['basement1'] = create_surrogate_basement(temp_surface_areas['footprint_area'], slab_constr)
        surfaces['slab'] = create_surrogate_slab(temp_surface_areas['footprint_area'], slab_constr)
        surfaces['slab1'] = create_surrogate_slab(temp_surface_areas['footprint_area'], slab_constr)

    # Do not have to add surrogate internal walls as those are added already in the idf file, but shear walls
    shear_constr = constr_list['Shear_wall-' + res_scenario].Name
    surfaces['shear_wall'] = create_surrogate_shear_wall(temp_surface_areas['floor_area_wo_basement'], shear_constr)
    if 'Shear_wall-' + res_scenario in [x for x in constr_list]:
        shear_constr = constr_list['Shear_wall-' + res_scenario].Name
        surfaces['shear_wall'] = create_surrogate_shear_wall(temp_surface_areas['floor_area_wo_basement'], shear_constr)

From what I understand, for any RES scenarios, these lines will be run:

    surfaces['basement'] = create_surrogate_basement(temp_surface_areas['footprint_area'], slab_constr)
    surfaces['basement1'] = create_surrogate_basement(temp_surface_areas['footprint_area'], slab_constr)
    surfaces['slab'] = create_surrogate_slab(temp_surface_areas['footprint_area'], slab_constr)
    surfaces['slab1'] = create_surrogate_slab(temp_surface_areas['footprint_area'], slab_constr)

    shear_constr = constr_list['Shear_wall-' + res_scenario].Name
    surfaces['shear_wall'] = create_surrogate_shear_wall(temp_surface_areas['floor_area_wo_basement'], shear_constr)
    if 'Shear_wall-' + res_scenario in [x for x in constr_list]:
        shear_constr = constr_list['Shear_wall-' + res_scenario].Name
        surfaces['shear_wall'] = create_surrogate_shear_wall(temp_surface_areas['floor_area_wo_basement'], shear_constr)

Then, only for RES scenarios 2.1 and 2.1+2.2, this must be run:

    if res_scenario == 'RES2.1' or res_scenario == 'RES2.1+RES2.2':
        floor_slab_constr = constr_list['Concrete_floor_slab-' + res_scenario].Name
        surfaces['int_floor_second_floor'] = create_surrogate_slab(temp_surface_areas['footprint_area'], floor_slab_constr)

My questions are: 1- Why are the slab and basement objects doubled for RT (slab, slab1 and basement, basement1)?

2- It is clear from the RT.idf that the Concrete_floor_slab- construction is only present in scenarios 'RES2.1+RES2.2'. However, could it be present in any other archetypes/scenarios combinations? If so, would it still be added to "int_floor_second_floor"? I'm trying to think of a general form for the function.

3- Shear walls seem to be added in two steps. Was this intended? This seems different from how 'attic-ceiling-', 'Surrogate_slab-', and 'Concrete_floor_slab-' are treated. The surfaces are first added in:

 shear_constr = constr_list['Shear_wall-' + res_scenario].Name
 surfaces['shear_wall'] = create_surrogate_shear_wall(temp_surface_areas['floor_area_wo_basement'], shear_constr)

And then they are modified (just after) in:

    if 'Shear_wall-' + res_scenario in [x for x in constr_list]:
        shear_constr = constr_list['Shear_wall-' + res_scenario].Name
        surfaces['shear_wall'] = create_surrogate_shear_wall(temp_surface_areas['floor_area_wo_basement'], shear_constr)

Shouldn't the shear walls only be added once in the if-loop, and not beforehand?

Thanks! (and sorry for the long post)

[nheeren]

Thanks for looking into this and for the critical reflection! As @anistad is the main author of the RT archetype and these functions, it would be great you could chime in here briefly. If Andrea does not find the time, please feel free to make some executive decisions 😉

1- Why are the slab and basement objects doubled for RT (slab, slab1 and basement, basement1)?

Not sure to be honest.

2- It is clear from the RT.idf that the Concrete_floor_slab- construction is only present in scenarios 'RES2.1+RES2.2'. However, could it be present in any other archetypes/scenarios combinations? If so, would it still be added to "int_floor_second_floor"? I'm trying to think of a general form for the function.

In general I think BuildME should be as generic as possible. Please feel free to propose something.

3- Shear walls seem to be added in two steps. Was this intended? This seems different from how 'attic-ceiling-', 'Surrogate_slab-', and 'Concrete_floor_slab-' are treated. The surfaces are first added in: ... Shouldn't the shear walls only be added once in the if-loop, and not beforehand?

Not sure again. I trust your careful judgement in this matter.

[anistad]

Hey!

I can have a look into this and check what's been going on. Buut I wont have time before next week..

Cheers, Andrea

søn. 17. apr. 2022, 13.20 skrev Niko Heeren @.***>:

Thanks for looking into this and for the critical reflection! As @anistad https://github.com/anistad is the main author of the RT archetype and these functions, it would be great you could chime in here briefly. If Andrea does not find the time, please feel free to make some executive decisions 😉

1- Why are the slab and basement objects doubled for RT (slab, slab1 and basement, basement1)?

Not sure to be honest.

2- It is clear from the RT.idf that the Concrete_floor_slab- construction is only present in scenarios 'RES2.1+RES2.2'. However, could it be present in any other archetypes/scenarios combinations? If so, would it still be added to "int_floor_second_floor"? I'm trying to think of a general form for the function.

In general I think BuildME should be as generic as possible. Please feel free to propose something.

3- Shear walls seem to be added in two steps. Was this intended? This seems different from how 'attic-ceiling-', 'Surrogate_slab-', and 'Concrete_floor_slab-' are treated. The surfaces are first added in: ... Shouldn't the shear walls only be added once in the if-loop, and not beforehand?

Not sure again. I trust your careful judgement in this matter.

— Reply to this email directly, view it on GitHub https://github.com/nheeren/BuildME/issues/57#issuecomment-1100855931, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMDENQUEJ5OHSUZQLMIOO3TVFPXV5ANCNFSM5R5UJIZA . You are receiving this because you were mentioned.Message ID: @.***>

[nheeren]

@CBreton026 So finally this issue has come back to bite me and is causing me a lot of headache, because many of the non-residential archetypes all use multipliers, which causes numerous issues. Would you mind making a PR or uploading the current state of your work into a branch?

I created a new branch (issue_57) to work on this.

Sorry -- I can't see it. Am I missing something?

Also I am sorry, I did not get back to you concerning to the pull requests. They both look fine and I will merge them.

[kamilakrych]

I've had a look at it. Indeed, the questions that Charles posed are very relevant. Particularly, why were the slab, basement and shear walls all doubled? Not sure.

Anyway, in my branch I have just uploaded a fix to this issue. I have deleted the double counting mentioned above, so the slab, basement are added as usual (this was present in the original "get_surfaces()"), and shear walls are added just to the RT and Office archetypes. The multiplier is integrated in the get_surfaces() and would multiply the elements if multiplier of more than 1 is encountered. Hope this helps!

[anistad]

Sorry for not following up on this earlier. According to the documentation for the RT, two levels of basement are assumed. Could this be why I doubled the basement and slab?

[kamilakrych]

I have added some fixes to the last fix :sweat_smile: So hopefully Kamila is no longer causing issues!

[CBreton026]

@nheeren I should have precised that the aforementioned branch (issue_57) was on my own BuildME fork.

Also, sorry for coming back to you so late. I just pushed the changes I had done to idf.py to the "issue_57" branch on my fork. What would be the best way to share them with you? Should I send a PR?

You should know that there were still lots of "Todos" and "Fixmes" in there; for example, I wasn't able to generalize the function for RT, nor did I change the treatment of shear walls. Nonetheless, I think there may be relevant snippets of code for you. When I worked on this, I tried to refactor some functions to make it easier to test / maintain afterwards.

[nheeren]

Thanks @CBreton026! I will review the changes by @kamilitsa first. If there should be open issues, I will dive deeper into your branch.