Support CxoTime in DateTime and modernize #42
Conversation
Chandra/Time/Time.py
Outdated
| time_in.sidereal_time | ||
| time_in.format | ||
| time_in.shape | ||
| return str(time_in.secs) |
There was a problem hiding this comment.
Not that I am opposed, but why do all these checks?
If you want to check that this is an astropy.time.Time instance, you can use isinstance. If what you want to check is the behavior, then isn't time_in.secs enough?
There was a problem hiding this comment.
The idea is to be extremely sure the object is an astropy Time subclass without actually importing astropy.time.Time. Just looking for a secs attribute is not strong enough because there are many unrelated classes that have a secs attribute. However almost no random classes will have all those attributes.
That is indeed a bit adhoc / hacky. Maybe there is a better way, like digging through .__class__.__mro__ to find 'astropy.time.core.Time'. I think that works, as long as astropy never refactors or changes the module name core.py.
There was a problem hiding this comment.
That reminds me the intent was to be slightly sneaky and actually use time_in.cxcsec to allow initialization from any astropy Time subclass since cxcsec is built-in. For initializing DateTime any Time object should be sufficient, but going the other way will result in a CxoTime coming out.
There was a problem hiding this comment.
Ah. Seems like both those thoughts ("without importing astropy.time" and "allow initialization from any subclass") could just make their way into the comment for now.
There was a problem hiding this comment.
But since this was a WIP...
There was a problem hiding this comment.
I removed the WIP. I need to check again that testing coverage is OK but I don't have any outstanding items for implementation in mind.
72e4f39 to
526a340
Compare
|
With apologies, I ended up migrating the unit tests to pytest in order to get things working. So this ends up making this into a multi-issue PR. |
08b7b94 to
a022d02
Compare
a022d02 to
d409652
Compare
|
I addressed the comments and simplified the implementation so it is much cleaner and more isolated from the original code. |
Description
Allow instantiating a DateTime object from a CxoTime object, and conversely getting a CxoTime object from DateTime using
tm.cxotime.One disappointing issue is that I discovered that CxoTime (astropy Time) is quite slow in creating scalar time objects, up to a factor of 15 slower (300 us vs. 20 us). For most applications this won't matter.
Testing
Fixes #