Skip to content

Support Time in table hstack, vstack and join - #6888

Merged
taldcroft merged 7 commits into
astropy:masterfrom
taldcroft:time-table-ops
May 3, 2018
Merged

taldcroft merged 7 commits into
astropy:masterfrom
taldcroft:time-table-ops

Conversation

@taldcroft

@taldcroft taldcroft commented Nov 21, 2017 •

Copy link
Copy Markdown
Member

This PR adds support for Time objects to be used in table operations hstack, vstack and join. It builds off the support for setting and masking in #6028.

The only substantive code change is in the new_like method to create a new empty Time object based on a list of input Time objects.

@taldcroft taldcroft added Affects-dev PRs and issues that do not impact an existing Astropy release table time Work in progress labels Nov 21, 2017
@astropy-bot

astropy-bot Bot commented Nov 21, 2017 •

Copy link
Copy Markdown

Hi there @taldcroft 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@taldcroft taldcroft added this to the v3.0.0 milestone Nov 21, 2017
@taldcroft taldcroft added Enhancement Affects-release and removed Affects-dev PRs and issues that do not impact an existing Astropy release Work in progress labels Dec 21, 2017
@taldcroft

Copy link
Copy Markdown
Member Author

@mhvk - this is ready for final review now, along with #6028. If you won't have time then let me know.

@bsipocz
bsipocz requested a review from mhvk December 21, 2017 16:38
@astrofrog astrofrog modified the milestones: v3.0.0, v3.1 Jan 23, 2018
@astrofrog

Copy link
Copy Markdown
Member

This is not to be backported to the v3.0.x branch as the coordinators decided this would need more testing before it makes it to a release.

@astrofrog

Copy link
Copy Markdown
Member

@taldcroft - now that #6028 is merged, can you rebase this?

@taldcroft

Copy link
Copy Markdown
Member Author

@astrofrog - yes, in fact I rebased already a few days ago, so there are now just 6 commits.

@taldcroft

Copy link
Copy Markdown
Member Author

The circleci fail is unrelated:

________________________________ test_2d_model _________________________________
[gw0] linux -- Python 3.6.4 /opt/python/cp36-cp36m/bin/python

    @pytest.mark.skipif('not HAS_SCIPY')
    def test_2d_model():
        # 2D model with LevMarLSQFitter
        gauss2d = models.Gaussian2D(10.2, 4.3, 5,2, 1.2, 1.4)
        fitter = fitting.LevMarLSQFitter()

Is this fixed in the most recent master?

@taldcroft

Copy link
Copy Markdown
Member Author

And of course I will fix the changelog to put into 3.1.

@astrofrog

Copy link
Copy Markdown
Member

Is this fixed in the most recent master?

I think so, I increased the tolerance in this test a couple of days ago

Comment thread astropy/time/core.py Outdated
input ``cols`` and has ``length`` rows.

This is intended for creating an empty column object whose elements can
be set in-place for table operations like join or vstack.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is in the astropy.time module, it might not be clear what columns refers to. Maybe saying 'multiple time objects' would make more sense? Or maybe explain more about the Table mixin functionality here.

@bsipocz

bsipocz commented May 1, 2018

Copy link
Copy Markdown
Member

@taldcroft @mhvk - I see this is ready for review for a while, do you have any update on it?

@taldcroft

Copy link
Copy Markdown
Member Author

@bsipocz - I have rebased again and addressed the review comments.

From my perspective this is good to go (pending tests of course).

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks all OK.

@taldcroft
taldcroft merged commit 9a2d341 into astropy:master May 3, 2018
@taldcroft

Copy link
Copy Markdown
Member Author

Thanks @mhvk!

@taldcroft
taldcroft deleted the time-table-ops branch May 3, 2018 16:27
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.

4 participants