-
Notifications
You must be signed in to change notification settings - Fork 122
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
Energy electron volt #273
Energy electron volt #273
Conversation
We can't add a unit group for them since they're different Dimensions. |
@derekmorr Wouldn't e.g. |
Oh, sorry. I misunderstood. I thought you meant adding units for I agree that naming will be an issue. |
5ba4a99
to
f03686f
Compare
lazy val TeV = TeraElectronVolt(1) | ||
lazy val PeV = PetaElectronVolt(1) | ||
lazy val EeV = ExaElectronVolt(1) | ||
lazy val evMultiplier: Double = 1.60217656535e-19 |
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.
Should this be moved into the ElectronVolt
object so we don't leak a constant?
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.
Yes, that's probably a good idea
05628d4
to
a48f924
Compare
This LGTM. Did we decide on how to handle naming issues? |
No decision yet, but I think it will be a problem to have the same unit name. I'm leaning to use WDYT? |
a48f924
to
4f2240a
Compare
@@ -187,6 +198,46 @@ object Ergs extends EnergyUnit { | |||
val symbol = "erg" | |||
} | |||
|
|||
object ElectronVolt extends EnergyUnit { | |||
val conversionFactor = Joules.conversionFactor * 1.60217656535e-19 |
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.
The conversion factor is wrong. The factor is defined as 1.602176565(35)x10e-19. The digits in the parentheses do not represent additional precision, however. Instead, that notation - known as uncertainty notation - represents the standard deviation of the last two digits in the value. It means it is ~65% likely that the conversion factor is actually 1.602176565e-19 +/- 0.000000035e-19, or ~95% likely that it is 1.602176565e-19 +/- 2*0.000000035e-19
The best factor for our purposes is 1.602176565e-19.
Perhaps in a future enhancement, we can account for this uncertainty somehow.
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 poiting this up, I'll update this and the constants PR that uses the same bad logic
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.
See my previous comment on the conversion factor.
7db9fe7
to
f0b09b9
Compare
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.
LGTM
Seems I could merge this. I'll wait a bit to let others chime in if needed |
I noted that we are missing the very important
Electron-Volt
unit for energy. This PR adds themI'll probably add the other uses of
eV
forMass
,Momentum
,Distance
andTemperature
Do you think I should add a
UnitGroup
for these?