Add 3D gaussian Gauss3 object
#29
Conversation
Codecov Report
@@ Coverage Diff @@
## main #29 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 13 +1
Lines 422 439 +17
=========================================
+ Hits 422 439 +17
Continue to review full report at Codecov.
|
| value::Number = 1, | ||
| ) | ||
| (cx, cy, cz, wx, wy, wz) = promote(cx, cy, cz, wx, wy, wz) | ||
| Object(Gauss3(), (cx,cy,cz), (wx,wy,wz), (Φ, Θ), value) |
There was a problem hiding this comment.
I guess this is already designed this way so there's no need to change. In some sense, it's just a matter of taste.
But generally speaking as a side comment, people would expect this constructor function to create a Gauss3 object, and the following fact would be a bit confusing from time to time:
Gauss3() isa Guass3 # true
Gauss3(...) isa Gauss3 # falseSpeaking of this particular issue, the lower case normal function gauss3 is more appropriate because it's allowed to return something not Gauss3.
There was a problem hiding this comment.
I really appreciate this feedback because I find it confusing too!
This package is still in 0.0.x stage so I am open to modifying it to make it more "usual."
I was attempting to follow what base does with round but probably I didn't match it properly:
https://github.com/JuliaLang/julia/blob/742b9abb4dd4621b667ec5bb3434b8b3602f96fd/base/rounding.jl#L52
Would you recommend simply changing to lower case gauss3 and ellipsoid etc., or is there a better overall way?
BTW, I still plan to merge this particular PR (because it matches the rest of the current package design), but I can consider your suggestion in a separate PR that could change it across the board.
You could add a note to the related issue I opened #30
And some minor tweaks elsewhere.