-
Notifications
You must be signed in to change notification settings - Fork 22
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
Adding Sphere object #75
Conversation
Initial class with basic functionality.
Hey @santiagogaray , Thanks for putting this together and I think there's a lot more we'd want to add like the following properties
... and methods:
Note that the only intrinsic attributes should be I'll do a longer code review after you have gotten the chance to implement some of these and write some tests for them. |
Thanks @chriswmackey for clarifying the initial functionality. Will start working on it! |
I updated the Sphere with the properties listed and a first take at the intersection methods. I have a few questions when you get a chance:
No rush Chris, whenever you can! |
Hey Santiago, Sorry for the late response. I'll do a full review shortly but I'll first answer your questions and highlight that your tests are failing in Python 3 because you have a Now to answer your questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this together @santiagogaray . I think if you can address my comments and write a few more tests, we will be able to merge this in.
Thanks for the detailed review @chriswmackey. |
- Fixing some code style issues - Fixing several documentation issues - Separating the math of the intersection functions out of the Sphere3D class as an intermeiate step before moving them to the intersection3D class.
Thanks again @chriswmackey. I've learned a few things from your review. I appreciate the time to explain things that clearly. I corrected almost everything . I included also the Point3D to_array and from_array functions for your review. The only change I am having problems with is moving the intersection functions to For your review, I thought would be better to add first a commit with the functions inside the |
…est error) pytest error collecting modules.
This last commit has the math moved to |
Thanks for addressing all of the comments, @santiagogaray . I will look over everything in detail shortly but I just wanted to say that the reason why the test is failing is because importing You will see that I have been solving this issue to-date by expressing the output of the intersections purely as
Then, on the I realize that this organization of the code seems a bit roundabout but, so far, I have found it helpful to have all of the intersection math stored in one place and, as you can see by all of the modules that import from the I am also cool if you just want to implement the intersection math on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Santiago. This is looking good and thanks for making the changes. Most of my comments are on typos in the docstrings. If you could take care of those and implement one of the solutions I mentioned for the intersection functions, we should be good to merge. Also, as you'll see in the comments, it would also be good if you can add rotate
and reflect
functions and I think these should be pretty straightforward.
If None, it will be scaled from the World origin (0, 0, 0). | ||
""" | ||
return Sphere3D(self.center.scale(factor, origin), self.radius * factor) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if you could include methods for rotate
and reflect
like the other geometry objects. I know that those two don't get used as much as the move
and scale
methods do but they have their usefulness and I am trying to ensure that all geometry objects support all 4 of the similar transformations:
- move
- scale
- rotate
- reflect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! With the sphere not having an orientation and wrongly assuming these where local transformations. Just needed to see the inputs, no excuse!
I added the rotate_xy since I see that is implemented in other classes as well. Hope that's okay.
Thanks @chriswmackey. I have to say I am ashamed on a few typos and oversights. I have to learn to to be more careful, and not rush on tasks that may look straightforward. I did all the changes listed (Hopefully!). I hope I didn't miss anything. Once this PR is closed I will be ready to start the next object. |
- Changing docstring typos -Adding missing transformation functions - Fixed intersection methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing all of the comments, @santiagogaray , and there's no need to be ashamed of typos. They are a natural part of developing code and one of the major reasons why we do code review. It's also another reason why we try to write tests for all of our code (usually we can surface a typo or two in the process of writing them). I am approving your PR since your code looks good enough to merge but one thing that would be really helpful before we merge is if you could write some tests for all of the methods on the Sphere3D
. You can see here that coveralls shows you the lines of your code that have tests for them and which ones lack tests. Personally, I try to get my code above 90% before I merge it in, though it's more important to write tests that meaningfully check your workflows than it is to get everything 100% covered.
As for your next PR, tackling the other objects (eg. cone, cylinder) would be great but I also wanted to note now that you have added the to_array
and from_array
methods in this PR, another really helpful task would be going through all the to_dict
and from_dict
methods on all of the geometry objects (except points and vectors) and replacing all of the code like this with your new array
methods. The call is up to you but I know that task might be a little quicker.
Thanks @chriswmackey. So much to learn, finally I understand the coverage test, extremely useful! I did bump the coverage to 98% and I actually found a bug in the copy function, so this proving to be a good strategy already. It definitely make sense to work on the |
Thanks for adding all of the tests, @santiagogaray and I am glad to hear the coverage is that high. I just have one small tweak that I would recommend in the way that you structured your tests, which will make it easy to identify what is failing if we ever make changes in the future. Specifically, the fact that you are testing so many things in a single function as you are here isn't the best practice since, if one of them fails, it's hard to tell where exactly the problem was (ie. does a failure in the moving of the object really mean a failure in this object init as your test name suggests). Breaking tests down to test individual features like you see here helps clarify this. Also one other note is that, if you want to test the string representation of an object, it's better practice to just write |
- Renaming test file to sphere3d_test - Separating Sphere3D tests by functions - Adding additional tests
Thanks @chriswmackey for this explanation. I am clearly very new to all these testing practices. I thought this was merely internal for me to check the code and maybe as a log of what has been done. Now I'm just realizing this is a test that will be run in future development of the module to make sure things stay bug-free. Is really great to finally understand this, loving the Github environment the more I understand it. |
Wonderful! Thank you for the quick edits, @santiagogaray . You have some very clean, well-documented code and tests here that should be able to stand the test of time! I'll merge this in now. |
@santiagogaray , it looks like you closed the pull request. Was this intentional or accidental (I know we were considering including some of the changes mentioned here in your next PR)? |
I just reopened it and I'll interpret the closing as accidental and merge it in unless you tell me not to within the next 5 minutes. |
Sorry @chriswmackey, a newbie move. Please go ahead and merge it in. |
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Initial class with basic functionality.
@chriswmackey
I created a bare bones Sphere class to get the PR started. I see in the project discussion that initially we wanted to add some functionality in line to the other ladybug-geometry classes. Additionally I saw you pointed out to this link for reference for math calculations:
https://github.com/ezag/pyeuclid/blob/master/euclid.py#L2033-L2114
This seems to focus on intersection type of algorithms. I wonder if it's this what we are after in terms of added functionality. Do we need to discuss what functionality is needed from this object?