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

Add respective frequency units for some time units. #63

Merged
merged 1 commit into from
Apr 13, 2018
Merged

Add respective frequency units for some time units. #63

merged 1 commit into from
Apr 13, 2018

Conversation

Aehmlo
Copy link
Contributor

@Aehmlo Aehmlo commented Apr 3, 2018

This is part of #30. I went ahead and added the inverse units for the existing (non-metric) time units, but stopped there.

@cycles_per_minute: 6.0_E1; "1/min", "cycle per minute", "cycles per minute";
@cycles_per_hour: 3.6_E3; "1/h", "cycle per hour", "cycles per hour";
@cycles_per_day: 8.64_E4; "1/d", "cycle per day", "cycles per day";
@cycles_per_year: 3.1536_E7; "1/a", "cycle per year", "cycles per year";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and used a as the abbreviation for the year here, following your lead from time.rs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these conversion factors should be inverted. e.g. 1 cycle per minute = 1.0 / 6.0_E1 Hz. Please double check my math however. If this is the case tests can also be added for Time <-> Frequency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…I knew something about that seemed off, but I kept convincing myself that "60 Hz is 1 cycle per minute, it's fine." I'll go fix that.

@iliekturtles
Copy link
Owner

  • Change the commit message to include "Resolves Frequency quantity #30." -- I don't think there are any other Frequency units need at this time.
  • Additional precision is needed for conversion factors.
  • Add tests.

I'll merge once the changes are made!

diff --git a/src/si/frequency.rs b/src/si/frequency.rs                                             
index 134c811..633b2a3 100644                                                                      
--- a/src/si/frequency.rs                                                                          
+++ b/src/si/frequency.rs                                                                          
@@ -36,11 +36,12 @@ quantity! {                                                                    
         @zeptohertz: prefix!(zepto); "zHz", "zeptohertz", "zeptohertz";                           
         @yoctohertz: prefix!(yocto); "yHz", "yoctohertz", "yoctohertz";                           
                                                                                                   
+        @cycles_per_day: 1.157_407_407_407_407_4_E-5; "1/d", "cycle per day", "cycles per day";   
+        @cycles_per_hour: 2.777_777_777_777_777_E-4; "1/h", "cycle per hour", "cycles per hour";  
+        @cycles_per_minute: 1.666_666_666_666_666_6E-2; "1/min", "cycle per minute",              
+            "cycles per minute";                                                                  
         @cycles_per_shake: 1.0_E8; "100 MHz", "cycle per shake", "cycles per shake";              
-        @cycles_per_minute: 1.666_666_E-2; "1/min", "cycle per minute", "cycles per minute";      
-        @cycles_per_hour: 2.777_777_E-4; "1/h", "cycle per hour", "cycles per hour";              
-        @cycles_per_day: 1.157_407_E-5; "1/d", "cycle per day", "cycles per day";                 
-        @cycles_per_year: 3.170_979_E-8; "1/a", "cycle per year", "cycles per year";              
+        @cycles_per_year: 3.170_979_198_376_458_E-8; "1/a", "cycle per year", "cycles per year";  
     }                                                                                             
 }                                                                                                 
                                                                                                   
@@ -82,6 +83,11 @@ mod tests {                                                                     
             test::<t::attosecond, f::exahertz>();                                                 
             test::<t::zeptosecond, f::zettahertz>();                                              
             test::<t::yoctosecond, f::yottahertz>();                                              
+            test::<t::day, f::cycles_per_day>();                                                  
+            test::<t::hour, f::cycles_per_hour>();                                                
+            test::<t::minute, f::cycles_per_minute>();                                            
+            test::<t::shake, f::cycles_per_shake>();                                              
+            test::<t::year, f::cycles_per_year>();                                                
                                                                                                   
             fn test<T: t::Conversion<V>, F: f::Conversion<V>>() {                                 
                 Test::assert_approx_eq(&(V::one() / Time::new::<T>(V::one())),                    

@Aehmlo
Copy link
Contributor Author

Aehmlo commented Apr 9, 2018

Third time's the charm! I think we should be good now.

One other thing: cycles_per_day doesn't strike me as consistent with e.g. gram or cubic_meter or ampere — namely, it's plural already. I went with this because it was the name suggested at the outset, but do we maybe want to go with e.g. cycle_per_day and update the others to match?

Just a thought.

@iliekturtles
Copy link
Owner

Good catch. Go ahead and make them singular and then I'll merge.

@iliekturtles iliekturtles merged commit e0aa5ad into iliekturtles:master Apr 13, 2018
@iliekturtles
Copy link
Owner

Merged. Thanks for the PR and sorry for the delay. I just realized that I don't receive a notification when you push new changes to the branch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants