PH-Tools / PHX

Passive House Exchange (PHX) model format and tools for model I/O
GNU General Public License v3.0
1 stars 3 forks source link

Speed up HBJSON_to_PHPP #47

Open gjtorikian opened 7 months ago

gjtorikian commented 7 months ago

👋 Hello! A friend of mine contacted me to see if I could help speed up his conversion of HBJSON to PHPP in Grasshopper. I traced the plugin back to your project, and was able to reproduce some of the performance issues locally.

In this PR, I propose two changes:

With this, I was able to drop a five minute run down to an average of a minute-thirty. Happy to make any additional changes necessary here! Thanks for keeping the project very well organized, it was easy to pull it apart and poke around.

ed-p-may commented 7 months ago

Hi @gjtorikian ! This is fantastic! Thank you so much for all this! I love it.

The one item I'm not 100% sure about is the async...await? I am not very familiar with the nuances of those operations, so I'm just not sure about one item: in some areas, it does matter which write comes first, and so I'm not sure if that would still work properly here?

Just as an example: the U-Values have to be written to the U-Values page first, before the Areas-Surfaces can be written, since each surface needs to know the worksheet location of their U-Value as they are written.... so for instance in the write_project_opaque_surfaces each surface-write goes and looks at the U-Values page to find the right item:

    def write_project_opaque_surfaces(self, phx_project: project.PhxProject) -> None:
        """Write all of the opaque surfaces from a PhxProject to the PHPP 'Areas' worksheet."""

        surfaces: List[areas_surface.SurfaceRow] = []
        for phx_variant in phx_project.variants:
            for opaque_component in phx_variant.building.opaque_components:
                for phx_polygon in opaque_component.polygons:
                    surfaces.append(
                        areas_surface.SurfaceRow(
                            self.shape.AREAS,
                            phx_polygon,
                            opaque_component,

                            #---
                            #--- Reads from U-Values worksheet here
                            #---
                            self.u_values.get_constructor_phpp_id_by_name(
                                opaque_component.assembly.display_name, _use_cache=True
                            #---
                            #---
                            ),
                        )
                    )

So I'm not sure, but it seems like perhaps if the U-Value write hasn't executed, this may cause an error? I'm not sure if that's how async... .await would affect this though? Perhaps it wouldn't in this case?

There are a couple spots like that: 'Ventilation' has to come before 'Spaces' Window Components' have to come before 'Windows' 'Windows' has to come before 'Shading'

For sure some like climate, lighting, appliances and others can be out of order and won't affect it though. I'll try and run some test scenarios and see what I find.

ed-p-may commented 7 months ago

One other item I notice on PHX/xl/xl_app.py: if we are maintaining a separate cached dictionary of the worksheet names now - then operations like create_new_worksheet will need to add to that dict as well. Otherwise things will get out of sync.

I think maybe the new worksheet_names would be better as a @property as well, otherwise it won't be in sync either if worksheets are potentially added to the dict during the excel interactions?

gjtorikian commented 7 months ago

in some areas, it does matter which write comes first, and so I'm not sure if that would still work properly here?

Shucks, I was worried about that. In those cases, yes, absolutely, async / await will break. We can't reliably determine the order of which write happens first using this method. Would it be possible to write out all of the known order dependencies ? I can figure out a solution where those write can happen sequentially while the others continue asynchronously.

Otherwise things will get out of sync. ... I think maybe the new worksheet_names would be better as a @property as well

Sounds good, I'll take care of it.

ed-p-may commented 7 months ago

Would it be possible to write out all of the known order dependencies ?

Good idea. They are all buried inside the /PHX/PHPP.phpp_app.PHPPConnection class. I think they would be:

1) write_project_opaque_surfaces must come after write_project_constructions 1) write_project_window_surfaces must come after write_project_opaque_surfaces 1) write_project_window_surfaces must come after write_project_window_components 1) write_project_window_shading must come after write_project_window_surfaces 1) write_project_ventilators must come after write_project_ventilation_components 1) write_project_spaces must come after write_project_ventilators

I think that's all of them actually? The rest are more or less independent I think.

Although: It's also possible this could all be re-designed as well? I was just trying to avoid maintaining a duplicate record of all the PHPP entries / locations / values, and so I was just using those 'read' operations in lieu of keeping track myself within the Python app. But if there is a significant performance reason, its certainly possible to add some form of local record keeping as well?