Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spacing of linspace can be more than step_max when supplied #45

Closed
jeanconn opened this issue Nov 5, 2024 · 12 comments · Fixed by #47
Closed

spacing of linspace can be more than step_max when supplied #45

jeanconn opened this issue Nov 5, 2024 · 12 comments · Fixed by #47
Labels

Comments

@jeanconn
Copy link
Contributor

jeanconn commented Nov 5, 2024

In [1]: import numpy as np
   ...: import astropy.units as u
   ...: from cxotime import CxoTime

In [2]: start = CxoTime('2023:001:00:00:01.000')
   ...: stop = CxoTime('2023:001:00:30:01.000')

In [3]: times = CxoTime.linspace(start, stop, step_max=60 * u.s)

In [4]: times[16] - times[15] > 60 * u.s
Out[4]: True

In [5]: times[15] - times[14] > 60 * u.s
Out[5]: False

I think to be completely correct for applications that require that the time deltas not be greater than step_max we'll need to add something to step_max when calculating the number of pieces, or just increment the number of pieces if any chunk is greater than step_max.

@jeanconn jeanconn added the bug label Nov 5, 2024
@taldcroft
Copy link
Member

I think you are just hitting some insignificant floating point rounding error here.

In [8]: times[16] - times[15] > 60.0000000001 * u.s
Out[8]: False
In [9]: times[16] - times[15] > 60.000000000 * u.s
Out[9]: True

@taldcroft
Copy link
Member

Or alternately:

In [15]: dt = times[16] - times[15]

In [16]: dt.jd2 * 86400
Out[16]: 60.00000000000458

@jeanconn
Copy link
Contributor Author

jeanconn commented Nov 6, 2024

I agree, yet if upstream tests use "<" it doesn't seem to matter if the interval is only larger by a floating point rounding error than step_max, the < still fails.

@taldcroft
Copy link
Member

I'm not sure where you are going with that. It's just the wrong test to apply, and e.g. np.linspace fails that same test.

In [28]: times = np.linspace(0, 3600 / 86400, 61)

In [29]: dt = 3600 / 86400 / 60

In [30]: dt
Out[30]: 0.0006944444444444444

In [31]: set(np.diff(times))
Out[31]: 
{0.000694444444444442,
 0.0006944444444444437,
 0.0006944444444444441,
 0.0006944444444444444,
 0.0006944444444444446,
 0.0006944444444444454,
 0.0006944444444444489}

In [32]: np.where(np.diff(times) > dt)
Out[32]: 
(array([ 3,  4,  6,  7,  8,  9, 12, 14, 17, 19, 22, 24, 25, 26, 28, 29, 31,
        32, 34, 35, 37, 38, 39, 41, 42, 44, 45, 48, 50, 53, 56, 59]),)

In [33]: np.where(np.diff(times) < dt)
Out[33]: 
(array([ 2,  5, 10, 11, 13, 15, 16, 18, 20, 21, 23, 27, 30, 33, 36, 40, 43,
        46, 47, 49, 51, 52, 54, 55, 57, 58]),)

@jeanconn
Copy link
Contributor Author

jeanconn commented Nov 6, 2024

OK, what is the appropriate test to confirm a time range isn't larger than the allowed maximum time range? Per your comments

sot/chandra_aca#181 (comment)

I thought it would be something like CxoTime(stop) - CxoTime(start) < Quantity, but apparently that's not what you have in mind.

https://github.com/sot/chandra_aca/blob/c48deacada26b7ad03e480ac0159afc9a22bfb05/chandra_aca/maude_decom.py#L1076

@taldcroft
Copy link
Member

I'm not sure of the context of your question. If it is in the reference chandra_aca maude_decom.py, then I think we're already done with:

MAUDE_SINGLE_FETCH_LIMIT = 3.0 * u.hour
MAUDE_FETCH_STEP_MAX = 3.0 * u.hour - 1.0 * u.second

It should not ever happen that a single fetch query exceeds MAUDE_SINGLE_FETCH_LIMIT given the 1 sec smaller step max.

If you are talking about a unit test here, then with your example you should just test for delta times < (60 + 1e-9) * u.s or something similar.

@jeanconn
Copy link
Contributor Author

jeanconn commented Nov 6, 2024

Sure, I thought the point of step_max was that the calling code could expect all the intervals to be smaller by a "<" test, so I considered it a bug and worked around it in my WIP by making step_max less than the limit. If you think it is fine that the intervals can be floating point error larger than step_max than we can go forward using that convention. I just figured you hated calling code to need to keep track of that kind of thing.

@taldcroft
Copy link
Member

Floating point comparisons always need to account for floating point errors. That's not unique to this application, as evidenced by the np.linspace result.

It never would even occur to me to check the thing you were checking in the calling code, I expect the function to work as advertised. You could update the linspace docs if need be to reflect that the intervals are less than step_max to within floating point precision.

@taldcroft
Copy link
Member

Basically, don't do that e.g.:

In [35]: x = np.pi + np.arange(500) * 0.123

In [36]: set(np.diff(x))
Out[36]: 
{0.12299999999999045,
 0.12299999999999756,
 0.12299999999999933,
 0.12299999999999978,
 0.12300000000000022,
 0.12300000000000111,
 0.12300000000000466}

@jeanconn
Copy link
Contributor Author

jeanconn commented Nov 6, 2024

Regarding "It never would even occur to me to check the thing you were checking in the calling code," I still don't understand because we headed along the path to creating the general CxoTime.linspace method specifically to break an interval into chunks so that each chunk would satisfy a time-limit requirement. So now we have a new method that doesn't satisfy the original requirement.

@taldcroft
Copy link
Member

Are you able to break the code at sot/chandra_aca#181 in any way?

If not then we are done. You are using the new CxoTime.linspace() in the intended/required way for this application by setting the linspace step_max to a value that is smaller than the MAUDE max fetch time.

@jeanconn
Copy link
Contributor Author

jeanconn commented Nov 6, 2024

I can't think of a way to break it with an explicit step_max smaller by any reasonable amount than the limit. And yes, sure, that's fine and that's what I meant by " If you think it is fine that the intervals can be floating point error larger than step_max then we can go forward using that convention."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants