Skip to content

Fix warnings in timeseries.periodograms#8964

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

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

Conversation

@bsipocz
Copy link
Member

@bsipocz bsipocz commented Jul 6, 2019 •
edited
Loading

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
Copy link
Member Author

bsipocz commented Jul 6, 2019 •
edited
Loading

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
astropy/timeseries/periodograms/lombscargle/implementations/cython_impl.pyx Outdated

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

t.flags.writeable = True
Copy link
Contributor

@mhvk mhvk Jul 7, 2019

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
Copy link
Contributor

mhvk commented Jul 7, 2019

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

@bsipocz
Copy link
Member Author

bsipocz commented Jul 7, 2019 •
edited
Loading

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
Copy link
Member

@pllim pllim left a comment

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
Copy link
Member

pllim commented Jul 8, 2019

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
Copy link
Member Author

bsipocz commented Jul 8, 2019

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
Copy link
Member Author

bsipocz commented Jul 15, 2019

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