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

Add some routines to get background subtracted images #174

Merged
merged 28 commits into from
Dec 3, 2024
Merged

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Aug 30, 2024

Description

Add some convenience routines to get dark-current background subtracted ACA images.

Interface impacts

Testing

This depends on sot/mica#301 and sot/mica#311 .

Unit tests

  • Mac
(ska3-flight-latest) flame:chandra_aca jean$ git rev-parse HEAD
6e20ccf80970ac9dad70a01e4008a5381d108d0c
(ska3-flight-latest) flame:chandra_aca jean$ python -c "import mica; print(mica.__version__)"
4.37.1.dev2+g8c56ac2
(ska3-flight-latest) flame:chandra_aca jean$ pytest
======================================================================= test session starts =======================================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
PyQt5 5.15.9 -- Qt runtime 5.15.8 -- Qt compiled 5.15.8
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: astropy-0.11.0, qt-4.4.0, cov-5.0.0, timeout-2.2.0, remotedata-0.4.1, anyio-4.3.0, filter-subpackage-0.2.0, doctestplus-1.2.1, astropy-header-0.2.2, hypothesis-6.112.0, arraydiff-0.6.1, mock-3.14.0
collected 231 items                                                                                                                                               

chandra_aca/tests/test_aca_image.py ...................                                                                                                     [  8%]
chandra_aca/tests/test_all.py ........................                                                                                                      [ 18%]
chandra_aca/tests/test_attitude.py .............................................................                                                            [ 45%]
chandra_aca/tests/test_dark_model.py ............                                                                                                           [ 50%]
chandra_aca/tests/test_dark_subtract.py ........                                                                                                            [ 53%]
chandra_aca/tests/test_drift.py ..........................                                                                                                  [ 64%]
chandra_aca/tests/test_maude_decom.py .........................                                                                                             [ 75%]
chandra_aca/tests/test_planets.py ...............                                                                                                           [ 82%]
chandra_aca/tests/test_psf.py ...                                                                                                                           [ 83%]
chandra_aca/tests/test_residuals.py ss...                                                                                                                   [ 85%]
chandra_aca/tests/test_star_probs.py .................................                                                                                      [100%]


====================================================== 229 passed, 2 skipped, 2 warnings in 65.07s (0:01:05

Independent check of unit tests by @taldcroft :

  • Mac
(masters) ➜  chandra_aca git:(back-subtract) git rev-parse --short HEAD                                              
37560e5
(masters) ➜  chandra_aca git:(back-subtract) pytest
================================================== test session starts ==================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 230 items                                                                                                     

chandra_aca/tests/test_aca_image.py ..................                                                            [  7%]
chandra_aca/tests/test_all.py ........................                                                            [ 18%]
chandra_aca/tests/test_attitude.py .............................................................                  [ 44%]
chandra_aca/tests/test_dark_model.py ............                                                                 [ 50%]
chandra_aca/tests/test_dark_subtract.py ........                                                                  [ 53%]
chandra_aca/tests/test_drift.py ..........................                                                        [ 64%]
chandra_aca/tests/test_maude_decom.py .........................                                                   [ 75%]
chandra_aca/tests/test_planets.py ...............                                                                 [ 82%]
chandra_aca/tests/test_psf.py ...                                                                                 [ 83%]
chandra_aca/tests/test_residuals.py ss...                                                                         [ 85%]
chandra_aca/tests/test_star_probs.py .................................                                            [100%]

============================================ 228 passed, 2 skipped in 40.30s ============================================

Functional tests

@jeanconn jeanconn marked this pull request as ready for review September 16, 2024 15:31
@jeanconn
Copy link
Contributor Author

@javierggt and @taldcroft this could use feedback on organization and names of methods etc.

@jeanconn
Copy link
Contributor Author

Actually it looks like I had some more changes in my local version, so I'll re-request review when those are up.

Base automatically changed from new-scaling to master September 16, 2024 16:04
@jeanconn jeanconn force-pushed the back-subtract branch 2 times, most recently from 2092874 to c5cca8e Compare September 17, 2024 16:09
@taldcroft
Copy link
Member

It's one of those days where I felt like spending too much time making some little code nicer. The hope is to take advantage of the power of CxoTime and u.Quantity, and solve a common problem of chunking a time interval.


def get_chunk_times(start: CxoTime, stop: CxoTime, dt_max: u.Quantity):
    """Get uniform time chunks for a given time range.

    Output times are spaced uniformly spaced by up to ``dt_max`` and cover the time
    range from ``start`` to ``stop``.

    Parameters
    ----------
    start : CxoTime
        Start time of the time range.
    stop : CxoTime
        Stop time of the time range.
    dt_max : u.Quantity (timelike)
        Maximum time interval for each chunk.

    Returns
    -------
    CxoTime
        CxoTime with time bins for each chunk.
    """
    # Calculate chunks to cover time range, handling edge case of start == stop
    n_chunk = max(np.ceil(float((stop - start) / dt_max)), 1)
    dt = (stop - start) / n_chunk
    times = start + np.arange(n_chunk + 1) * dt
    return times


@retry.retry(exceptions=requests.exceptions.RequestException, delay=5, tries=3)
@lru_cache(maxsize=2)
def get_maude_images(start: CxoTimeLike, stop: CxoTimeLike, fetch_limit=100 * u.hr):
    """
    Get ACA images from MAUDE for a given time range.

    Parameters
    ----------
    start : CxoTimeLike
        Start time of the time range.
    stop : CxoTimeLike
        Stop time of the time range.
    fetch_limit : u.Quantity (timelike)
        Maximum number of hours to fetch from MAUDE in one go.

    Returns
    -------
    dict
        dict with a astropy.table of ACA image data for each slot.
    """
    start = CxoTime(start)
    stop = CxoTime(stop)

    if stop - start > fetch_limit:
        raise ValueError("Time range too large for maude fetch, increase 'fetch_limit'")

    chunks = []
    # Break maude fetches into max 3 hour chunks required by maude_decom fetch
    times = get_chunk_times(start, stop, 3 * u.hr)
    for mstart, mstop in zip(times[:-1], times[1:]):
        imgs = maude_decom.get_aca_images(mstart, mstop)
        chunks.append(imgs)
    chunks = vstack(chunks)

    slot_data = {}
    for slot in range(8):
        slot_data[slot] = chunks[chunks["IMGNUM"] == slot]

    return slot_data

For example:

In [8]: get_chunk_times(CxoTime("2024:001"), CxoTime("2024:002"), 3.5 * u.hr)
Out[8]: 
<CxoTime object: scale='utc' format='date' value=['2024:001:00:00:00.000' '2024:001:03:25:42.857' '2024:001:06:51:25.714'
 '2024:001:10:17:08.571' '2024:001:13:42:51.429' '2024:001:17:08:34.286'
 '2024:001:20:34:17.143' '2024:002:00:00:00.000']>

@taldcroft
Copy link
Member

taldcroft commented Sep 20, 2024

@jeanconn - big picture comment about API. What about sticking with the maude_decom.get_aca_images standard of a single Table of images and related columns. Then include a helper function to convert that into a dict of Table keyed by slot. I think this might be more general instead of enforcing a new data structure (and one that I don't commonly use).

For existing code I have that grabs ACA images from MAUDE, I'd like an easy way to use this new code to do the dark subtraction on my existing table of images. So that might mean passing in a table of images and having the function return a new table which is matched to the input and contains relevant outputs, most importantly the dark-subtracted image.

@taldcroft
Copy link
Member

It also occurs to me that we might want to upstream the 3-hr chunking and time limit here to maude_decom.

@jeanconn
Copy link
Contributor Author

It looks like your concept of "get_chunk_times" is pretty general - where should that function live?

@jeanconn
Copy link
Contributor Author

With regard to " What about sticking with the maude_decom.get_aca_images standard of a single Table of images and related columns." - sure - when I wanted the data by slot I figured a compromise between the maude_decom.get_aca_images standard (has one table but a column for slot) and the mica.archive.aca_l0.get_slot_data standard (underlying data is in files by slot so the fetch works by slot) made sense, but if you prefer to use a table with slot as a column that's fine.

@jeanconn
Copy link
Contributor Author

jeanconn commented Sep 20, 2024

Regarding "I'd like an easy way to use this new code to do the dark subtraction on my existing table of times." - is that a table of times or a table of aca images? I'm a little confused about which piece you want to plug in and where - do you want to point me to which code you had in mind?

@taldcroft
Copy link
Member

I meant "table of images from maude_decom.get_aca_images".

The point of a more atomic API (accepting a single table of images) is that if your application is working with a dict of tables, it is trivial to do this in your app:

for slot, image_table in image_table_dict.items():
    images_dark_sub = ... # table of dark subtracted images for that slot
    # Do something with it

Whereas if I have a single table with multiple slots included then that can also work with the same functions. If the functions required a dict of table then that wouldn't work.

@taldcroft
Copy link
Member

It looks like your concept of "get_chunk_times" is pretty general - where should that function live?

Hmm, your guess is as good as mine. ska_helpers.utils is there as a last resort to dump anything.

@jeanconn
Copy link
Contributor Author

Right, but wouldn't it make just as much sense to replace the call to maude_decom.get_aca_images with a call to whatever we call this?

@jeanconn
Copy link
Contributor Author

Regarding "get_chunk_times" I was thinking cxotime first though I suppose it isn't specific to cxotimes.

@jeanconn jeanconn mentioned this pull request Nov 29, 2024
2 tasks
@taldcroft
Copy link
Member

@jeanconn - I added a few commits. Most importantly I fixed a problem in the tests, and then chose a new time range which causes the tests to now fail (a real bug I believe). Also a bunch of docstring updates for more consistency and a bit more concise.

chandra_aca/dark_subtract.py Outdated Show resolved Hide resolved
col8x8 = row["IMGCOL0_8X8"]

# If row8x8 or col8x8 is < 0 just skip the test for now
if row8x8 < 0 or col8x8 < 0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I just cheated a bit here as I didn't want to deal with negative indexes in the test.

# If row8x8 or col8x8 is < 0 just skip the test for now
if row8x8 < 0 or col8x8 < 0:
# If row8x8 or col8x8 is < -512 just skip the test for now
if row8x8 < -512 or col8x8 < -512:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I have an update to handle the "off the negative CCD edges", but I don't think that we can get real data like that at all, so maybe there is no point.

This improves performance substantially.
@taldcroft
Copy link
Member

It turns out that having that numba function inline within the get_dark_backgrounds function was causing the numba jit compilation to happen every time. That is because of the size variable which is out of scope of the numba function. I was playing with profiling and noticed a constant 100 ms overhead regardless of how many cutouts were requested.

So I fixed that and then saw that the numba function could be cleaned up as well.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

I think this is good to go!!

@jeanconn jeanconn merged commit 139c22c into master Dec 3, 2024
2 checks passed
@jeanconn jeanconn deleted the back-subtract branch December 3, 2024 16:41
@javierggt javierggt mentioned this pull request Dec 9, 2024
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