Precise vector test #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added file for edge clipping
* implemented shape * removed private interface * updated * implemented shape * removed spaces, kept eq impl
yjh0502
requested changes
Jun 18, 2019
| } | ||
|
|
||
| // PreciseVectorFromVector creates a high precision vector from the given Vector. | ||
| pub fn precise_vector_from_vector(v: r3::vector::Vector) -> PreciseVector { |
There was a problem hiding this comment.
Could we use impl From<r3::vector::Vector> for PriciseVector?
| } | ||
|
|
||
| // NewPreciseVector creates a high precision vector from the given floating point values. | ||
| pub fn new_precise_vector(x: f64, y: f64, z: f64) -> PreciseVector { |
There was a problem hiding this comment.
How about
impl PreciesVector {
fn new(x: f64, y: f64, z: f64) -> Self { ... }
}
| impl PreciseVector { | ||
|
|
||
| // Vector returns this precise vector converted to a Vector. | ||
| pub fn vector(&self) -> r3::vector::Vector { |
| } | ||
|
|
||
| // Equal reports whether v and ov are equal. | ||
| pub fn equal(&self, ov: PreciseVector) -> bool { |
There was a problem hiding this comment.
How about std::cmp::Eq trait? (or PartialEq?)
| } | ||
|
|
||
| // Add returns the standard vector sum of v and ov. | ||
| pub fn add(&self, ov: PreciseVector) -> PreciseVector { |
There was a problem hiding this comment.
Could we use std::ops::Add and friends?
| } | ||
|
|
||
| // Sub returns the standard vector difference of v and ov. | ||
| pub fn sub(&self, ov: PreciseVector) -> PreciseVector { |
| } | ||
|
|
||
| // Mul returns the standard scalar product of v and f. | ||
| pub fn mul(&self, f: bigdecimal::BigDecimal) -> PreciseVector { |
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
|
|
||
|
|
||
| // sortEdges sorts the slice of Edges in place. | ||
| pub fn sort_edges(mut e: Vec<Edge>) { |
There was a problem hiding this comment.
How about pub fn sort_edges(e: &mut [Edge])? The function takes Vec<Edge> and drops the vector after sort, so there's no way to get a sorted result.
| // OriginReferencePoint returns a ReferencePoint with the given value for | ||
| // contained and the origin point. It should be used when all points or no | ||
| // points are contained. | ||
| pub fn origin_reference_point(contained: bool) -> ReferencePoint { |
There was a problem hiding this comment.
Could we put this function into impl ReferencePoint { ... }?
|
@haynes1 Any feedback so far? May I merge the PR and apply feedbacks by myself? I'm a bit worried that it might break your code as it changes API. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All but one test pass.
The test that doesn't pass is test_precise_round_trip(). This test converts a f64 vector to a precise vector, then converts it back to an f64 vector and normalizes it. The result should be equivalent to just normalizing the original f64 vector but it isn't.
The problem is that after conversion, the post-conversion normalized vector is accurate to one more decimal place than the pre-conversion normalized vector.
pre: 0.2672612419124244 post: 0.26726124191242434
Potential Research:
rust-lang/rust#24557
https://stackoverflow.com/questions/50361151/how-to-deal-with-inexact-floating-point-arithmetic-results-in-rust