Skip to content

Stop setting copy to False by default#55

Merged
taldcroft merged 3 commits intomasterfrom
unset-copy-default
Dec 9, 2025
Merged

Stop setting copy to False by default#55
taldcroft merged 3 commits intomasterfrom
unset-copy-default

Conversation

@jeanconn
Copy link
Copy Markdown
Contributor

@jeanconn jeanconn commented Dec 3, 2025

Description

Don't set default of copy=False in __init__.

In numpy > 2 the copy=False default causes an error if the operation cannot be done without a copy. In numpy < 2.0 this was benign as copy=False was equivalent to "copy if you have to". But changing to not set the default appears to be benign in ska with numpy < 2.0 and the astropy Time default was already to not-copy so a change here appears to be non-impacting.

Interface impacts

Testing

Unit tests

  • Mac on ska3-latest (numpy < 2)
(latest) flame:cxotime jean$ git rev-parse HEAD
59d104b70102f4ea55c14969aa65edfcef33b3f2
(latest) flame:cxotime jean$ pytest
============================= test session starts ==============================
platform darwin -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: anyio-4.7.0, timeout-2.3.1
collected 292 items                                                            

cxotime/tests/test_cxotime.py .......................................... [ 14%]
........................................................................ [ 39%]
........................................................................ [ 63%]
........................................................................ [ 88%]
...................                                                                                                                                       [ 94%]
cxotime/tests/test_cxotime_linspace.py ...............                                                                                                    [100%]

====================================================================== 292 passed in 1.63s

Independent check of unit tests by Javier

  • OSX:
(ska3-flight-2026.0rc4) ~/SAO/git/cxotime unset-copy-default $ git rev-parse HEAD       
a00bd8e183b26a8783cc350615376a15242b304f
(ska3-flight-2026.0rc4) ~/SAO/git/cxotime unset-copy-default $ pytest cxotime           
==================================================================== test session starts =====================================================================
platform darwin -- Python 3.13.10, pytest-9.0.1, pluggy-1.6.0
rootdir: /Users/javierg/SAO/git
configfile: pytest.ini
plugins: anyio-4.12.0, timeout-2.4.0
collected 292 items                                                                                                                                          

cxotime/tests/test_cxotime.py ........................................................................................................................ [ 41%]
...................................................................................................................................................... [ 92%]
.......                                                                                                                                                [ 94%]
cxotime/tests/test_cxotime_linspace.py ...............                                                                                                 [100%]

==================================================================== 292 passed in 1.52s =====================================================================
(ska3-flight-2026.0rc4) ~/SAO/git/cxotime unset-copy-default $ conda deactivate                    
(ska3-flight) ~/SAO/git/cxotime unset-copy-default $ pytest cxotime  
==================================================================== test session starts =====================================================================
platform darwin -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0
rootdir: /Users/javierg/SAO/git
configfile: pytest.ini
plugins: anyio-4.7.0, timeout-2.3.1
collected 292 items                                                                                                                                          

cxotime/tests/test_cxotime.py ........................................................................................................................ [ 41%]
...................................................................................................................................................... [ 92%]
.......                                                                                                                                                [ 94%]
cxotime/tests/test_cxotime_linspace.py ...............                                                                                                 [100%]

==================================================================== 292 passed in 1.60s =====================================================================

Functional tests

No functional testing.

Comment thread cxotime/cxotime.py Outdated
Comment thread cxotime/cxotime.py Outdated
jeanconn and others added 2 commits December 3, 2025 14:44
Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
@jeanconn
Copy link
Copy Markdown
Contributor Author

jeanconn commented Dec 9, 2025

@taldcroft @javierggt should we configure this to be backward compatible using numpy version testing?

@javierggt
Copy link
Copy Markdown
Contributor

Tests pass with this version on flight. What happens on flight if the default is not set?

@javierggt
Copy link
Copy Markdown
Contributor

javierggt commented Dec 9, 2025

What Tom was suggesting yesterday was to use astropy.utils.compat.NUMPY_LT_2_0 (I checked that it is defined on flight, so it is not a problem to do it.

Comment thread cxotime/cxotime.py
@jeanconn
Copy link
Copy Markdown
Contributor Author

jeanconn commented Dec 9, 2025

Right, Tom was suggesting "astropy.utils.compat.NUMPY_LT_2_0" for use in a different package, but we could use it here. I don't know if it is strictly required that we preserve the copy False behavior for numpy < 2.0 because it wasn't clear to me if numpy was actually managing to do the operation without a copy. But maybe it is lowest impact to just do the if statement.

@javierggt
Copy link
Copy Markdown
Contributor

I am guessing that Tom put that statement there because otherwise it would be copied. This is a performance thing.

Unfortunately, no test fails on flight when I remove the line, so I would have to think which case that was intended for.

@javierggt
Copy link
Copy Markdown
Contributor

javierggt commented Dec 9, 2025

I have tried a few things to get a test to fail in flight when I remove that statement, and no luck. I don't know what corner case this is meant for.

@taldcroft
Copy link
Copy Markdown
Member

To be honest I do not recall why I put that there. The upstream default (in astropy Time) is already to not copy, so I believe this is currently a no-op in flight. Things might have been different when I wrote CxoTime.

In any case, we should not clutter the code with this conditional since tests are currently passing in flight.

@taldcroft
Copy link
Copy Markdown
Member

@jeanconn jeanconn marked this pull request as ready for review December 9, 2025 17:00
@taldcroft taldcroft merged commit a4594b1 into master Dec 9, 2025
2 checks passed
@taldcroft taldcroft deleted the unset-copy-default branch December 9, 2025 17:35
This was referenced Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants