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 an electron unit #2599

Merged
merged 3 commits into from Jun 10, 2014
Merged

Add an electron unit #2599

merged 3 commits into from Jun 10, 2014

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented Jun 5, 2014

This is used heavily in ccdproc; having it in core astropy would be convenient. Right now some CCD-related units are in astropy (e.g. u.adu) but electron ends up being ccdproc.electron.

ccdproc already registers electron with u.add_enabled_units, so the only time this comes up at all is creating a Quantity by doing something like q = 3 * ccdproc.electron. The lengthier form q = u.Quantity(3, "electron") works.

This may well be something fixable on the ccdproc side -- if so, please let me know.

@mhvk
Copy link
Contributor

mhvk commented Jun 5, 2014

I wondered whether one couldn't use photon, but ended up concluding it is good to add this, as one may want to convert to photon rate via an instrumental efficiency (and in X-ray astronomy, the two have more distinct meaning, with the number of electrons corresponding to the energy of the photon). So, 👍. Only comment would be to put it under "EVENTS" just above (and indeed move photon there too, and move pix down to miscellaneous...).

@mwcraig
Copy link
Member Author

mwcraig commented Jun 5, 2014

Should adu also move up to counts?

@embray
Copy link
Member

embray commented Jun 5, 2014

Would it be worth including an equivalency with electric charge and such?

@mwcraig
Copy link
Member Author

mwcraig commented Jun 5, 2014

I don't think so, because this means electron as a count rather than its physical properties.

@mdboom
Copy link
Contributor

mdboom commented Jun 5, 2014

👍 from me. As an aside, we may want to also propose this to the VOUnit working group for inclusion in the standardization effort so other tools can read/write this unit.

@mhvk
Copy link
Contributor

mhvk commented Jun 5, 2014

Thinking more about the events category, I think I'd just remove the category and stuff both photon, ct, pix, in miscellaneous. All of them at some level are not proper units at all, but rather tell you something that is being counted. (I guess it could also be "enumerable items" or so, but miscellaneous seems fine.)

@embray
Copy link
Member

embray commented Jun 5, 2014

In this case it does, but you could definitely take a count of electrons and convert it to charge. But that doesn't make any sense in the case of a detector or anything where there's a time dependence.

In other words, given a charge you could convert it to a rough number of electrons. Just a thought though--I know that doesn't really relate to the original purpose of this.

@cdeil
Copy link
Member

cdeil commented Jun 5, 2014

I'm working with data from photomultipliers and there the term photoelectron is used for what is meant by electron here, i.e. an electron registered in the detector in response to a photon.

But I agree plain electron is probably simpler and better, so 👍 to merge this now (maybe add an extra comment line in the code that it's supposed to be used for counting).

@cdeil
Copy link
Member

cdeil commented Jun 5, 2014

One of the travis-ci builds needs to be re-started.

@mwcraig
Copy link
Member Author

mwcraig commented Jun 5, 2014

To collate all of the comments so far:

  • Move all of the things in the EVENTS section to miscellaneous (to be clear, the sections are just divided by comments so this doesn't change anything in the API or docs)
  • Keep the name electron... @cdeil, do you want photoelectron in as an alternative name? That is easy to add (photon is also ph, so electron could be the short name for photoelectron and this would clarify perhaps that it is meant as a count rather than a charge).

@cdeil
Copy link
Member

cdeil commented Jun 5, 2014

Yes, I would like photoelectron as an alternative name.
But if someone finds a reason why this is a bad idea I can use electron in my code, no problem.

@mdboom
Copy link
Contributor

mdboom commented Jun 6, 2014

It seems to me that the name photoelectron is better in the event that we add an electron in the future that, as @embray suggested, it might make sense to convert to a charge. I think we really are talking about the more specific term (photoelectron) than the general term here (electron). I just would hate to paint ourselves into a corner here if we need the general electron in the future. (Keep in mind these names don't just end up being used in Python code, where the change might be relatively easy to make, but into written out file formats where interchange makes it very hard to change a convention later).

@mwcraig
Copy link
Member Author

mwcraig commented Jun 6, 2014

@crawfordsm -- any strong opinions on this? It seems like photoelectron would capture what we want in ccdproc, though things like read noise are typically expressed in electrons...

@crawfordsm
Copy link
Member

Electron is far more appropriate as that is what is actually being read out. The voltage measured on the device is being converted to number of electrons and that is what is recorded.

Photoelectron would assume that all of your electrons are due to the photoelectric effect. But since many devices (especially photomultiplier tubes and warm CCDs) have a dark current, some of the electrons are due to thermal effects and not photons. So it is actually more accurate to use electrons than photoelectrons and they are not strictly interchangeable.

I'm not sure if I know of any reason that electron should not be directly convertible to a charge.

@cdeil
Copy link
Member

cdeil commented Jun 6, 2014

OK ... 👍 to electron as the main term.
I guess photoelectron as an alternative name would be OK for applications where the electrons originate from photons?

Actually with the current implementation is electron convertible to a charge?
(I agree it should)

@mdboom
Copy link
Contributor

mdboom commented Jun 6, 2014

Ok -- thanks for the detailed response. That's all making sense now that maybe electron is fine. To answer @cdeil's question: In the present implementation electron isn't convertible to anything (it's an "island" unit). We can change that in the future by providing an equivalency function for that.

@embray
Copy link
Member

embray commented Jun 6, 2014

👍

@astrofrog
Copy link
Member

I think electron is fine.

By the way, why should it be made equivalent to a charge and not a mass? In fact, it could be made equivalent to both via equivalencies. E.g. (3 * u.electron).to(u.g, equivalency=...) uses the electron mass, while (3 * u.electron).to(u.C, equivalency=...) converts to a charge. I think that either way, electron is fine and we don't need a different name for the unit that is equivalent to a charge vs a mass (but the equivalency could be different).

@mhvk
Copy link
Contributor

mhvk commented Jun 6, 2014

Agreed to handle conversions via equivalencies (which can be added later). In the present context, the main use would seem exactly that the unit is an island unit, and that, therefore, you will have to divide by something like e-/(W/m**2 nm) to get to a physical unit.

Typing the above reminds me: I would add format={'latex': r'e^{-}', 'unicode': 'e⁻'}

@mwcraig
Copy link
Member Author

mwcraig commented Jun 6, 2014

Ended up adding the formatting but not making any other changes...

@astrofrog
Copy link
Member

@mwcraig - can you add a changelog entry? (we can sneak it in to 0.4)

@astrofrog astrofrog added this to the v0.4.0 milestone Jun 7, 2014
This is used heavily in ccdproc; having it in core astropy would be convenient
@mwcraig
Copy link
Member Author

mwcraig commented Jun 7, 2014

Done - getting it into 0.4 would be great!

astrofrog added a commit that referenced this pull request Jun 10, 2014
@astrofrog astrofrog merged commit 6daf290 into astropy:master Jun 10, 2014
@mwcraig mwcraig deleted the add-electron-unit branch August 19, 2014 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants