search

ladybug-tools / ladybug-geometry

🐞 📦 A library with geometry objects used throughout the Ladybug Tools core libraries
https://www.ladybug.tools/ladybug-geometry/docs/
GNU Affero General Public License v3.0
26 stars 23 forks source link

Change to_dict and from_dict methods using Point arrays to use a to_array and from_array method on the Point classes #77

Closed chriswmackey closed 4 years ago

chriswmackey commented 5 years ago

This is a simple change that will make the to_dict and from_dict code cleaner across all of the geometry objects.

A lot of the dictionary representations of objects represent point data as arrays of 2 or 3 values, such as that which you see here for Face3D: https://github.com/ladybug-tools/ladybug-geometry/blob/master/ladybug_geometry/geometry3d/face.py#L150

A cleaner way to do this is to add a to_array method to the Point2D and Point3D classes along with a from_array classmethod. These methods could then be called in the to_dict and from_dict methods of each of the other objects that represent point data as arrays. This should minimize the amount of duplicate code that we have across all of the geometry objects, which is basically just converting the point object to an array.

santiagogaray commented 4 years ago

Hi @chriswmackey This is ready, but don't want to commit until we close the other PR. One comment, I placed the from_array and to_array functions in the Vector2D and Vector3D classes since I see we can use this in vectors as well. It is inherited by the point classes. Hope this makes sense!

chriswmackey commented 4 years ago

@santiagogaray ,

Thanks for taking care of this. As long as your changes here don't involve any changes to the Sphere3D class, there actually won't be any conflicts if you send another PR alongside your current one. In other words, when you send a PR, Github is only merging in the changes to files; not the files themselves. Still, I realize that jumping back and forth between two copies of the code on your local machine can get annoying. Do you want to just split up your Sphere3D tests as part of your new PR and we can merge your old one in?

chriswmackey commented 4 years ago

The other PR has been merged. Feel free to send the new one once you get the chance to rebase your fork and sommit your local changes.

chriswmackey commented 4 years ago

This issue was addressed via https://github.com/ladybug-tools/ladybug-geometry/pull/78