Add support for time-series data #241

devang-chauhan · 2021-08-25T13:35:57Z

I have prettified the modules and they are ready for review.
I will still have to add unit tests after your review.
Also note that in this iteration, only one data can be used for time steps. For example, sDA and UDI cannot be loaded at the same time. I think we're close but I will have to work on it in the next iteration.

resolves #223

coveralls · 2021-08-25T13:37:29Z

Pull Request Test Coverage Report for Build 1180799700

178 of 311 (57.23%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-4.2%) to 74.108%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
honeybee_vtk/model.py	15	17	88.24%
honeybee_vtk/vtkjs/schema.py	14	16	87.5%
honeybee_vtk/cli/translate.py	18	24	75.0%
honeybee_vtk/load_config.py	79	86	91.86%
honeybee_vtk/time_series.py	49	165	29.7%

Totals
Change from base Build 1180797746:	-4.2%
Covered Lines:	1517
Relevant Lines:	2047

💛 - Coveralls

devang-chauhan · 2021-08-27T13:04:03Z

honeybee_vtk/vtkjs/schema.py

+            raise ValueError(
+                f'{v} is not a valid value for the "type" field. '
+                'Only the default value is accepted.'
+            )


@mostaphaRoudsari,
I am sure, you'll ask me to use Enum here. But I have deliberately not done that.

devang-chauhan · 2021-08-27T13:06:06Z

honeybee_vtk/time_series.py

+        animation = Animation(
+            timeSteps=[AnimationTimeStep(time=time_step, Grid=grid_obj)
+                       for time_step in time_steps])
+        data['animation'] = animation.dict()


@mostaphaRoudsari,
I am creating a dictionary here and that's the reason I am not using Enum in schema. I get json serialization error if I use Enum here.

devang-chauhan · 2021-08-27T13:07:39Z

honeybee_vtk/time_series.py

+        data['animation'] = animation.dict()
+
+    with open(index_json, 'w') as file:
+        json.dump(data, file)


I am aware that I can bypass the error by using json.dumps here dut I want to avoid doing that. Since incase of thousands of time steps this data here will be huge.

Sounds good! I didn't really leave a comment on this module. I think once we do a refactor of honeybee-radiance output files most of these functionalities will change anyways.

mostaphaRoudsari

Hi @devngc! Thank you for the PR and sorry for the late review.

My only strong comment is about changing to_vtkjs to a staticmethod. I think we should keep to_vtkjs as the main method for exporting the model to vtkjs. Other than that it looks good to me.

mostaphaRoudsari · 2021-09-12T02:11:54Z

honeybee_vtk/cli/translate.py

@@ -60,20 +60,29 @@ def translate():
    show_default=True
 )
 @click.option(
-    '--validate-data', '-vd', is_flag=True, default=False,
+    '--validation', '-vd', is_flag=True, default=False,


Since this is a flag I think --validate is a better option that --validation

mostaphaRoudsari · 2021-09-12T02:13:32Z

honeybee_vtk/cli/translate.py

    help='Validate simulation data before loading on the model. This is recommended'
    ' when using this command locally.', show_default=True
 )
+@click.option(
+    '--time-series', '-ts', is_flag=True, default=False,
+    help='Load time series data on a honeybee-vtk moodel.', show_default=True


Suggested change

help='Load time series data on a honeybee-vtk moodel.', show_default=True

help='Indicate data input is time series data.', show_default=True

mostaphaRoudsari · 2021-09-12T02:14:59Z

honeybee_vtk/cli/translate.py

+    if time_series and file_type != 'vtkjs':
+        raise ClickException(
+            'Time series data can only be exported to vtkjs. Use the file-type options'
+            ' to set "vtkjs" as the output file type.')


Suggested change

' to set "vtkjs" as the output file type.')

' to set the output file type to "vtkjs".')

mostaphaRoudsari · 2021-09-17T14:58:46Z

honeybee_vtk/load_config.py

+        grid_type: A string indicating whether the model has points and meshes.
+    """
+    # file path to the json file
+    grids_info_json = pathlib.Path(data.path).joinpath('grids_info.json')


What you have done is totally fine but I think this is prettier!

Suggested change

grids_info_json = pathlib.Path(data.path).joinpath('grids_info.json')

grids_info_json = pathlib.Path(data.path, 'grids_info.json')

mostaphaRoudsari · 2021-09-17T15:01:58Z

honeybee_vtk/load_config.py

+            f'In data {data.identifier.capitalize()}, since min and max'
+            ' values are not provided, those values will be auto calculated based'
+            ' on data. \n'


It feels a bit wordy what about this:

Suggested change

f'In data {data.identifier.capitalize()}, since min and max'

' values are not provided, those values will be auto calculated based'

' on data. \n'

f'Legend min and max values are autocalculated for {data.identifier.capitalize()}.\n'

Also I don't know if we want to give this warning at all. It might confuse our users more than being helpful.

mostaphaRoudsari · 2021-09-17T15:04:18Z

honeybee_vtk/load_config.py

+        raise TypeError(
+            'Not a valid json file.'
+        )
+    else:


This comment is mostly for making the code pretty.

I would not use else here since there is no follow-up section in the code. if it doesn't create an exception then the rest of the code should be executed anyways.

mostaphaRoudsari · 2021-09-17T15:06:28Z

honeybee_vtk/load_config.py

+            if not data.hide:
+                if not time_series:
+                    result = _get_model_with_point_in_time_data(
+                        validation, data, model, grid_type, scene, legend)
+                else:
+                    result = to_time_series_folder(data, model, grid_type, hbjson)


This is a bit of a code smell. The fact that we need both the Model and the Scene to load the data and config doesn't feel right.

Maybe this method should be called from inside the Scene or Model object? Let's see how it works.

mostaphaRoudsari · 2021-09-17T15:07:22Z

honeybee_vtk/model.py

@@ -260,37 +261,29 @@ def _add_objects(self, data: Dict) -> None:
            except AttributeError:
                raise AttributeError(f'Invalid attribute: {attr}')

-    def to_vtkjs(self, folder: str = '.', name: str = None) -> str:
-        """Write a vtkjs file.
+    def to_point_in_time_folder(self, target: pathlib.Path = None) -> pathlib.Path:


Should this be an internal method?

mostaphaRoudsari · 2021-09-17T15:10:36Z

honeybee_vtk/model.py

+        return point_in_time_folder
+
+    @staticmethod
+    def to_vtkjs(temp_folder: pathlib.Path, folder: str = '.', name: str = None) -> str:


This is not a good move in my opinion from the API point of view. to_vtkjs is what we have used as the standard method to write the model to a vtkjs file. Making it a static second-level method is not what I would do.

In any case, it is strange to have temp_folder as a required input. Moreover we should either use strings or pathlib.Path for folder inputs. It looks so random that the first one is expecting a Path and the second folder is a string.

mostaphaRoudsari · 2021-09-17T15:13:33Z

honeybee_vtk/time_series.py

+        data['animation'] = animation.dict()
+
+    with open(index_json, 'w') as file:
+        json.dump(data, file)


Sounds good! I didn't really leave a comment on this module. I think once we do a refactor of honeybee-radiance output files most of these functionalities will change anyways.

devang-chauhan marked this pull request as draft August 25, 2021 13:39

devang-chauhan marked this pull request as ready for review August 27, 2021 13:02

devang-chauhan requested a review from mostaphaRoudsari August 27, 2021 13:02

devang-chauhan commented Aug 27, 2021

View reviewed changes

devang-chauhan added 20 commits August 30, 2021 09:13

refactor(model): split to_vtkjs method

a6d0e17

style(model): update docstring

5977523

fix(config): support all object_type rather than only grid

770cf2a

feat(load_config): separate module for load_config

12d5161

refactor(*): update import statements

1026609

feat(time_series): add support for time_steps

d26d440

add(schema): add schemas to be used in time_series

e7a8a19

refactor(translate): update translate command to support time_steps

1a5bf92

style(config): update import statement

fbc8a4f

refactor(load_config): migrate function out of the module

28181fd

refactor(model): update to_vtkjs method and rename _to_folder method

cc936b4

test(*): update unit tests

b2b2e27

refactor(schema): add validator and other style changes

a269e07

style(schema): add developer comments

cd72d91

refactor(translate): improve workflow

00e75ac

refactor(load_config): simplify load_config function

bff08b4

refactor(model): better variable names

c81939d

refactor(time_series): better variable names and workflow

597a7bc

refactor(schema): stop sub-classing IndexJSON schema

af54a18

test(config_test): update tests

3de33b5

mostaphaRoudsari reviewed Sep 17, 2021

View reviewed changes

devang-chauhan closed this by deleting the head repository Sep 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for time-series data #241

Add support for time-series data #241

devang-chauhan commented Aug 25, 2021 •

edited

coveralls commented Aug 25, 2021 •

edited

devang-chauhan Aug 27, 2021

devang-chauhan Aug 27, 2021

devang-chauhan Aug 27, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari left a comment

mostaphaRoudsari Sep 12, 2021

mostaphaRoudsari Sep 12, 2021

mostaphaRoudsari Sep 12, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari Sep 17, 2021

mostaphaRoudsari Sep 17, 2021

	help='Load time series data on a honeybee-vtk moodel.', show_default=True
	help='Indicate data input is time series data.', show_default=True

	' to set "vtkjs" as the output file type.')
	' to set the output file type to "vtkjs".')

	grids_info_json = pathlib.Path(data.path).joinpath('grids_info.json')
	grids_info_json = pathlib.Path(data.path, 'grids_info.json')

Add support for time-series data #241

Add support for time-series data #241

Conversation

devang-chauhan commented Aug 25, 2021 • edited

coveralls commented Aug 25, 2021 • edited

Pull Request Test Coverage Report for Build 1180799700

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mostaphaRoudsari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devang-chauhan commented Aug 25, 2021 •

edited

coveralls commented Aug 25, 2021 •

edited