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

Handle filling aca image array differently #297

Merged
merged 3 commits into from
Mar 29, 2024
Merged

Handle filling aca image array differently #297

merged 3 commits into from
Mar 29, 2024

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Mar 22, 2024

Description

Handle filling aca image array differently

Fixes #293

Interface impacts

Testing

Unit tests

  • Linux
ska3-jeanconn-fido> git rev-parse HEAD
a1ca210964cbf815a3adce67bcd5323723b6f9c9
ska3-jeanconn-fido> pytest
================================================================= test session starts ==================================================================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: anyio-4.3.0, timeout-2.2.0
collected 108 items                                                                                                                                    

mica/archive/tests/test_aca_dark_cal.py ..................                                                                                       [ 16%]
mica/archive/tests/test_aca_hdr3.py .                                                                                                            [ 17%]
mica/archive/tests/test_aca_l0.py ...                                                                                                            [ 20%]
mica/archive/tests/test_asp_l1.py .......                                                                                                        [ 26%]
mica/archive/tests/test_cda.py ..............................................                                                                    [ 69%]
mica/archive/tests/test_obspar.py .                                                                                                              [ 70%]
mica/report/tests/test_write_report.py .                                                                                                         [ 71%]
mica/starcheck/tests/test_catalog_fetches.py ...............                                                                                     [ 85%]
mica/stats/tests/test_acq_stats.py ...                                                                                                           [ 87%]
mica/stats/tests/test_guide_stats.py ....                                                                                                        [ 91%]
mica/vv/tests/test_vv.py .........                                                                                                               [100%]

=========================================================== 108 passed in 517.20s (0:08:37)

Independent check of unit tests by Javier

  • Linux

Functional tests

No functional testing.

@jeanconn jeanconn requested review from javierggt and taldcroft March 22, 2024 16:56
for i, row in enumerate(dat):
imgraw = imgraws[i].reshape(8, 8)
sz = imgsizes[i]
if sz < 8:
imgraw = imgraw[:sz, :sz]

meta = {name: col[i] for name, col in cols.items() if col[i] != -9999}
meta = {name: col[i] for name, col in cols.items() if col[i] != fill_vals[name]}
Copy link
Contributor

Choose a reason for hiding this comment

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

imgraw.fill_value is not -9999, but you filled it with -9999, so the condition if col[i] != fill_vals[name] will never be true for IMGRAW. Will that be a problem? Or IMGRAW is an exception?

It might be better to do

dat["IMGRAW"].fill_value = -9999
imgraws = dat["IMGRAW"].filled()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the ambiguity is that I only made this work for the the default columns (IMGRAW isn't in the default columns) and it isn't expected that one would put the 8x8 imgraw data in the metadata for the image. I should probably just remove imgraw from columns if it is in there.

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 your idea to update the fill value for IMGRAW is also a good one. Technically I think I'd need to do that before making that little dictionary I have from column names to fill values to be 100% consistent (but the idea would still be that we don't have to worry about imgraw in the "add meta data to the ACAImage section").

Copy link
Member

@taldcroft taldcroft Mar 26, 2024

Choose a reason for hiding this comment

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

I think you don't need to do any filling at all except for imgraws. For the meta values for each row do:

names = dat.dtype.names
for i, row in ...:
    meta = {name: row[name] for name in names if not dat.mask[name][i]}

Technically speaking if dat["IMGSIZE"] is masked then that entire row is junk and should just be skipped. Using a filled value for that column would lead to undefined behavior.

Copy link
Member

@taldcroft taldcroft Mar 26, 2024

Choose a reason for hiding this comment

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

And probably the whole thing can be nicer as:

# Only keep rows with non-masked IMGSIZE
dat = dat[~dat["IMGSIZE"].mask]
for row,  imgraw in zip(dat, imgraws):
    imgraw = imgraw.reshape(8, 8)
    sz = row["IMGSIZE"]
    if ..:
        ...
    meta = {name: row[name] for name in names if not np.any(row.mask[name])}
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need to go and find missing data to see which of these filters was needed in general. IIRC IMGSIZE is a calculated quantity from another part of the code, so I didn't see a way that it could have a masked value, for example

Copy link
Member

Choose a reason for hiding this comment

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

I think that my 2nd suggestion just works in any case and you don't need to spend time digging. It's all going to be fast enough as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your second suggestion doesn't seem to unmask IMGRAW, and I thought the larger point was not to let the masked array end up as the ACAImage still masked?

Copy link
Member

@taldcroft taldcroft Mar 26, 2024

Choose a reason for hiding this comment

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

That was assuming your change imgraws = dat["IMGRAW"].filled(-9999). (Note the first sentence in https://github.com/sot/mica/pull/297/files#r1539513631).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I think I addressed the comments here.

@jeanconn jeanconn requested a review from javierggt March 28, 2024 16:40
Copy link
Contributor

@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I checked the unit test (including mica/archive/tests/test_aca_l0.py::test_get_l0_images) and verified that the deprecation warning does not show.

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.

LGTM!

@jeanconn jeanconn merged commit 4aca4cf into master Mar 29, 2024
@jeanconn jeanconn deleted the filled branch March 29, 2024 12:52
This was referenced Apr 17, 2024
@javierggt javierggt mentioned this pull request May 1, 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.

DeprecationWarning: NumPy will stop allowing conversion of out-of-bound Python integers to integer arrays
3 participants