Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 opened this issue Nov 11, 2019 · 4 comments
Labels
done enhancement New feature or request geometry good first issue Good for newcomers ladybug

Comments

@chriswmackey
Copy link
Member

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
Copy link
Contributor

santiagogaray commented Nov 19, 2019

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member Author

This issue was addressed via #78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done enhancement New feature or request geometry good first issue Good for newcomers ladybug
Projects
None yet
Development

No branches or pull requests

2 participants