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

Fits event lists #2

Closed
wants to merge 42 commits into from
Closed

Fits event lists #2

wants to merge 42 commits into from

Conversation

matteobachetti
Copy link
Collaborator

Introduces a new FITS reader class, that lazy loads the data until a slice is required (in which case, the wanted StingrayTimeseries object is created).

@matteobachetti matteobachetti marked this pull request as draft August 6, 2024 00:05
@matteobachetti matteobachetti force-pushed the fits_event_lists branch 9 times, most recently from 196405f to d6c8782 Compare September 2, 2024 15:09
@matteobachetti matteobachetti marked this pull request as ready for review September 2, 2024 15:13
Copy link
Collaborator

@eleonorav89 eleonorav89 left a comment

Choose a reason for hiding this comment

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

Nice work, I added just some comments :) I hope this is somehow useful

--------
>>> gtis = [[0, 30], [50, 60], [80, 90]]
>>> new_gtis = split_gtis_at_indices(gtis, 1)
>>> assert np.allclose(new_gtis[0], [[0, 30]])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to change with a clearer line showing the result/return in the example. assert np.allclose(... is kind of tricky to understand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a little tricky at the moment after Numpy 2.0. They changed the repr of numpy arrays, so that, e.g. when you print out the value of the array in the doctest it appends np.float64 stuff, and if you had just put some [[0, 30]] as the expected output, the test would fail. If you take it one step further and you just say, e.g. >>> np.allclose(new_gtis[0], [[0, 30]]) it will not say True, but np.True_ 😠
See, e.g.: astropy/astropy#15095
Astropy went for showing the updated format, but this means we cannot test, e.g., in Numpy 1.26.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I see, then let's leave it like this.

stingray/gti.py Show resolved Hide resolved
stingray/gti.py Outdated Show resolved Hide resolved
stingray/gti.py Outdated Show resolved Hide resolved
stingray/gti.py Outdated Show resolved Hide resolved
stingray/gti.py Show resolved Hide resolved
stingray/gti.py Outdated

exposure_edges = np.asarray(exposure_edges)

total_exposure = last_exposure
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would leave the starting total_exposure as different for the last_exposure. It could be useful for the use to check their work maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They serve different purposes: the initial total_exposure was an approximate measurement. I hope the new naming clarifies this difference

@matteobachetti
Copy link
Collaborator Author

Merged upstream! StingraySoftware#834

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.

3 participants