Skip to content

Time max, min, ptp, sort - #3681

Merged
taldcroft merged 6 commits into
astropy:masterfrom
mhvk:time-max-min
Jun 29, 2015
Merged

taldcroft merged 6 commits into
astropy:masterfrom
mhvk:time-max-min

Conversation

@mhvk

@mhvk mhvk commented Apr 13, 2015

Copy link
Copy Markdown
Contributor

Following on #3224, this adds argmin, argmax, max, min, and ptp methods to Time. In order to properly propagate location and the two delta attributes, min and max internally use argmin and argmax to get indices of the location that should be extracted.

@taldcroft - as this builds on #3224, you may want to either just look here, or just look at specific commit adding these methods.

EDIT: also added argsort and sort.

@mhvk mhvk added this to the v1.1.0 milestone Apr 13, 2015
@mhvk mhvk changed the title Time max, min, ptp Time max, min, ptp, sort Apr 13, 2015
@mhvk
mhvk force-pushed the time-max-min branch 2 times, most recently from b0805f6 to 3a2cf8a Compare June 21, 2015 16:45
@mhvk

mhvk commented Jun 21, 2015

Copy link
Copy Markdown
Contributor Author

@taldcroft - now also updated the max, min, etc. Missing still is mean, which needs a bit of thought on how to keep full precision.

@mhvk

mhvk commented Jun 21, 2015

Copy link
Copy Markdown
Contributor Author

@taldcroft - could you have a quick look at the travis failures? I think they occur because Time now has min, max, but am a little unsure...

@mhvk

mhvk commented Jun 21, 2015

Copy link
Copy Markdown
Contributor Author

OK, found the problem. Because I had not defined an out keyword, np.min(t) did not work any more. Now it does again, though it meant adding an out keyword that cannot actually be used.

@mhvk

mhvk commented Jun 26, 2015

Copy link
Copy Markdown
Contributor Author

@taldcroft - and also the sequel to the shape-changing is ready for final review...

@taldcroft

Copy link
Copy Markdown
Member

@mhvk - can you rebase this to get rid of the redundant commits? I think the commits from #3224 would not appear now except that #3224 was rebased.

@mhvk

mhvk commented Jun 26, 2015

Copy link
Copy Markdown
Contributor Author

@taldcroft - ok, rebased. I must have not quite kept track of all the rebasing!

@taldcroft

Copy link
Copy Markdown
Member

@mhvk - put in a PR on your branch. Otherwise this looks good to me.

@mhvk

mhvk commented Jun 29, 2015

Copy link
Copy Markdown
Contributor Author

@taldcroft - I looked at your and my argmin code and at first couldn't convince myself either was absolutely correct. But then I remembered my original logic, which was that really one wants to do

<some_guess>
dt = (self - self.__class__(<some_guess>, format='jd')).jd
return dt.argmin()

and looking at __sub__ again, it is clear my original logic should be fine. I now added this as comments to argmin and argmax.

While looking at this again, however, I felt somewhat uncomfortable with argsort, as it really presumes the jd1 and jd2 have gone through day_frac correctly. Of course, this should be the case, but just to be sure, I rewrote it in a way that should be fail-safe for that case too (though a little slower, but since argsort is so slow anyway, I don't think it matters too much).

p.s. Included your test additions with a minor fixup to the way the parametrization was set up.

@taldcroft

Copy link
Copy Markdown
Member

OK, I'm convinced! I should have remembered itertools.product, good catch.

taldcroft added a commit that referenced this pull request Jun 29, 2015
@taldcroft
taldcroft merged commit e05afbb into astropy:master Jun 29, 2015
@mhvk
mhvk deleted the time-max-min branch June 29, 2015 13:33
@mhvk mhvk mentioned this pull request Aug 12, 2015
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.

3 participants