Time candidate for merge #332
Conversation
This options system now provides TimeFormat subclasses with a dict container to hold options that control behavior. The only current example is "precision". This is implemented as a dict subclass that inherits from a global options dict at the time of __getitem__. This class does key and value validation for the allowed options.
This commit adds the ability to specify and select variations on a TimeString-based format (e.g. TimeISO). It also adds the capability to parse heterogenous input time formats. The input values must still all parse within a single TimeFormat class. These named subformats allow for input parsing of related but different formats, e.g. '%Y-%m-%d %H:%M:%S', '%Y-%m-%d %H:%M', and '%Y-%m-%d'. The allowed subformats can be selected by name using unix globbing of the subformat names. The above three are named 'date_hms', 'date_hm', and 'date', so one can require at least date and hour and minute with 'date_hm*'. Likewise for output one can select subformats using a glob, though the first selected subformat is used.
Use a few hardwired attributes/properties instead of 'opt' dict. This makes things more natural and cleaner for the user. If too many more attributes get added then this may get ugly but for now it is better (and realistically there probably won't be more than a couple more).
|
@cdeil -
There is a bit of a reason here. The But your idea is good and in ddd5df0 I added a In [4]: t = Time('B1950.0', scale='utc', precision=3)
In [5]: t.val
Out[5]: 'B1950.000'
In [6]: t.vals
Out[6]:
array(['B1950.000'],
dtype='|S9') |
|
@taldcroft - I see you put in a bunch of commits a few days ago... Is this ready to merge as far as you are concerned? And I see you generated some issues based on the earlier comments - between that and these changes, has everything brought up so far here been addressed? (Just so we're all on the same page) |
|
@eteq - yes all the comments here are either addressed or have been migrated to an issue. So it's ready to merge from my perspective. |
|
This looks good to me. I have a very minor comment, but not an issue if this gets merged first - there are a couple of instances where @taldcroft used |
|
@astrofrog and @eteq - fixed the XXX issue and found a couple more little problems after looking at the documentation once more. I'm sure there are some lurking issues but they have not surfaced yet... |
|
I think we should merge this pretty soon, and if you find any issues with the code/docs, just submit a PR. In the end, we should get people to try using this once merged to see if they run into any issues. As far as I'm concerned, this can be merged. @eteq? |
|
@taldcroft - given that this is a large PR could you push this to your staging branch for testing? |
|
@mdboom and @astrofrog - I just started jenkins builds on debian and macosx for this PR. A few days ago I pushed a different branch to staging and waited about an hour for builds to automatically start, but no go. So I started builds manually. Is that the expected behavior now? Just want to make sure I'm doing things right. |
|
@taldcroft - just for the record, you should never start the MacOS X build manually. It will just try and build it with a debian environment and we sometimes need to go and clean out the workspace manually. Same goes for the Windows builds. The debian ones are the only ones that can/should be started from Shining Panda. On another note, my Jenkins machine is refusing to boot (it crashed) so I'm not able to run the MacOS X tests at the moment. |
|
I think the last time I wait on MacOS for at least an hour before kicking off manually. |
|
Rats, there are a bunch of Python 3 fails that I need to investigate and resolve. |
|
@taldcroft - the MacOS X machine was down, hence why there were no results after 30 minutes. It's back up now, and should publish tests soon. What I meant was, the button to start the MacOS X and Windows builds does not actually work, and at best will do nothing, at worst will prevent builds from being published. The publishing only goes one way, the 'build' button never communicates with my mac. |
|
The MacOS X results are online, and there are indeed Python 3 failures. You might want to check in case there are any failures that aren't in the Linux one. Once you push changes, it should take 30-40 minutes to publish, so be patient :-) |
- Encode string/unicode to UTF8 before passing to sofa_time routines. - Fix test fail because python3 repr of float has more digits.
| original object and then converting the internal time values in the copy to the | ||
| new time scale. The new |Time| object is returned by the attribute access. | ||
|
|
||
| Transformation deltas |
There was a problem hiding this comment.
Maybe avoid calling these "deltas" to avoid confusion with the "Time Deltas" section title? Instead perhaps "Time scale transformation offsets"?
|
@taldcroft - now that all the tests are passing on @taldcroft's staging, this looks great to me aside from the very minor bits below (and the one comment I just left inline). Thanks for all the hard work!
|
|
@taldcroft - looks great to me... are we all ready to merge now? |
|
As far as I know we are ready to merge. I don't have anything outstanding. |
|
Ok, I've merged this PR. @taldcroft - thanks for the great contribution! |
Time candidate for merge
Remove register command
This represents a candidate for merging and supercedes PR #294. New features since the last commit 0abdeb7 on the time branch there:
set/get_delta_*methods into property attributes (f131694)Of particular note for the API is that the proposed standard for users will be:
This provides all "user" functionality for the time package. I think this is clean and looks nice. Developers wishing to write derived classes etc will do:
Still to do prior to merge: