Skip to content

Fix warnings in timeseries.periodograms - #8964

Merged
bsipocz merged 2 commits into
astropy:masterfrom
bsipocz:timeseries_ls_fix_warnings
Jul 8, 2019
Merged

Fix warnings in timeseries.periodograms#8964
bsipocz merged 2 commits into
astropy:masterfrom
bsipocz:timeseries_ls_fix_warnings

Conversation

@bsipocz

@bsipocz bsipocz commented Jul 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

This is part of efforts of #7928, as well as to fix numpy 1.17 compatibility as noted in #8935

@bsipocz bsipocz added this to the v2.0.15 milestone Jul 6, 2019
@bsipocz
bsipocz requested a review from mhvk July 6, 2019 19:46
@pllim pllim mentioned this pull request Jul 6, 2019
Merged
@bsipocz

bsipocz commented Jul 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

While Jake's cython was easily readable, I'm not sure I grasp what's going on in the python/cython/c layers for BLS, so one deprecation warning remains in there.
Also, this raises the issue that not all cases are covered in tests as the only deprecation is coming from the doctesting of the narrative docs.

@mhvk - could you please advise/push a fix to this branch?

mhvk
mhvk reviewed Jul 7, 2019
View reviewed changes
Comment thread astropy/timeseries/periodograms/lombscargle/implementations/cython_impl.pyx Outdated

t, y, dy = np.broadcast_arrays(t, y, dy)

t.flags.writeable = True

@mhvk mhvk Jul 7, 2019

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'm not sure it is right to just make the arrays writable - if any values are broadcast, something like y -= ... could very easily do the wrong thing. Will check.

@mhvk

mhvk commented Jul 7, 2019

Copy link
Copy Markdown
Contributor

Looking into this more, this really seems an upstream issue, in numpy and/or cython; see numpy/numpy#13929

@bsipocz

bsipocz commented Jul 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Thanks @mhvk for hunting down the upstream issue. Once #8965 is merged I go ahead and remove the superfluous commit from here and rebase. Edit: rebase is done.

@bsipocz
bsipocz force-pushed the timeseries_ls_fix_warnings branch from f48bbfe to a6231dc Compare July 7, 2019 20:08
@bsipocz bsipocz removed the numpy-dev label Jul 8, 2019
pllim
pllim approved these changes Jul 8, 2019
View reviewed changes

@pllim pllim left a comment

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.

Looks uncontroversial enough. Thanks!

@pllim

pllim commented Jul 8, 2019

Copy link
Copy Markdown
Member

I am afraid straight backport to 2.x might not be possible due to the refactoring though. Are you sure you want to milestone this to 2.x? Aside from the milestone, I think this is good to merge.

@bsipocz

bsipocz commented Jul 8, 2019

Copy link
Copy Markdown
Member Author

Things just got moved around so git should recognize it, but I will remilestone if it turns out to be problematic.

@bsipocz
bsipocz merged commit 8ad81e0 into astropy:master Jul 8, 2019
@mhvk mhvk mentioned this pull request Jul 8, 2019
Merged
@bsipocz

bsipocz commented Jul 15, 2019

Copy link
Copy Markdown
Member Author

Affected file has been added in 3.0, so this cannot be backported to LTS.

@bsipocz bsipocz modified the milestones: v2.0.15, v3.2.2 Jul 15, 2019
bsipocz added a commit that referenced this pull request Jul 15, 2019
@bsipocz
Merge pull request #8964 from bsipocz/timeseries_ls_fix_warnings
db8870c 
Fix warnings in timeseries.periodograms
@bsipocz
bsipocz deleted the timeseries_ls_fix_warnings branch September 16, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk left review comments
@pllim pllim pllim approved these changes

Assignees

No one assigned

Labels

no-changelog-entry-needed timeseries

Projects

None yet

Milestone

v3.2.2

Development

Successfully merging this pull request may close these issues.

3 participants

@bsipocz @mhvk @pllim