-
Notifications
You must be signed in to change notification settings - Fork 101
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
Changed default toa errors for event files. can also set explicitly #1591
Conversation
It's sort of weird to have errors associated with individual photons, but I agree they probably need to be > 0 to make basic fitting/chi^2 calculations work. |
Looks good to me! Thanks for the quick fix! |
Oops, the CI doesn't like the repeated |
Yes, I fixed that. I was actually thinking that an explicit test of this for non-Fermi satellites would also be useful (I hadn't had time/found the right test yet). For the Fermi case it is tested. |
@dlakaplan The time tagging of Fermi photons is only guaranteed to about that level, though, right? So there really is an error. @paulray probably knows the exact number. |
Fermi LAT webpage says "<10us, relative to spacecraft time" |
Smith et al. (2008) says: "Ground tests using cosmic ray muons demonstrated that the LAT measures event times with precision relative to UTC significantly better than a microsecond (Smith et al. 2006). On orbit, satellite |
I guess my point was more about hard-coding a single value for all spacecraft, rather than having some uncertainty present. But this is fine in any case. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 68.07% 68.13% +0.05%
==========================================
Files 98 98
Lines 22608 22618 +10
Branches 3881 3883 +2
==========================================
+ Hits 15391 15410 +19
+ Misses 6270 6260 -10
- Partials 947 948 +1
☔ View full report in Codecov by Sentry. |
I made all of the uncertainties separate and set by instrument. It would actually be better to turn the instrument-by-instrument functions into some factory functions. |
Looks good! I might be better to make a factory function, but I don't think it needs to be part of this PR. I'm in favor of merging this once the tests pass. |
I can probably use |
It's pretty easy using
Do you think this is OK? The docstring can still be correct. This is easier to update/maintain. |
Let me know if you think the latest push is OK. It's easier than before, but you still need to set each function explicitly (more complicated things are possible but seemed harder). It's a lot easier to read the definitions and make sure they are all consistent. The downside is that the signature is harder to understand. |
Yeah, that looks good. It is nice to remove all the redundancy. |
The other side of this is that the functions created with |
Oh, that's not optimal |
Yes, I'll see if there's a quick fix. |
Oddly, sphinx-doc/sphinx#5305 says it will add autodocs for partial functions, and I think our sphinx should be recent enough. But on the other hand I see people in the original issues trying with 1.8 and still having problems. I don't know if there were divergences between the sphinx 1.8 releases (which we use) and the 2.0/3.0/... releases. We are pinned to <2, but the current release is 7 |
@paulray : please look at the lated RTD build. I explicitly added the instrument-specific functions. It's still not perfect, as they don't collapse like normal. And now the instruments need to be included in 3 places. I still feel that something like this is useful, but maybe it's better to go back a couple of pushes and deal with this other stuff later. |
I think this looks fine. My only suggestion would be to try to get the preferred |
Changed |
It is possible to define these functions dynamically. But I haven't found a nice way. The best I can do is:
which seems too messy. |
Default errors are 1 us for all event files. Can also set as a
Quantity
orfloat