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

Fix data type when getting a line offset for a segmented hrit_jma #2930

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

k3a
Copy link
Contributor

@k3a k3a commented Oct 15, 2024

Fixes the following uint8 overflow when loading Himawari hrit data distributed by EumetCast:

Traceback (most recent call last):
  File "/Work/meteo/satpy-tests/./test5.py", line 18, in <module>
    scn = Scene(filenames=filenames, reader='ahi_hrit')
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/scene.py", line 155, in __init__
    self._readers = self._create_reader_instances(filenames=filenames,
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/scene.py", line 176, in _create_reader_instances
    return load_readers(filenames=filenames,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/__init__.py", line 580, in load_readers
    reader_instance.create_storage_items(
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/yaml_reader.py", line 617, in create_storage_items
    return self.create_filehandlers(files, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/yaml_reader.py", line 1180, in create_filehandlers
    created_fhs = super(GEOSegmentYAMLReader, self).create_filehandlers(
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/yaml_reader.py", line 629, in create_filehandlers
    filehandlers = self._new_filehandlers_for_filetype(filetype_info,
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/yaml_reader.py", line 612, in _new_filehandlers_for_filetype
    return list(filtered_iter)
           ^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/yaml_reader.py", line 594, in filter_fh_by_metadata
    for filehandler in filehandlers:
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/yaml_reader.py", line 590, in _new_filehandler_instances
    yield filetype_cls(filename, filename_info, filetype_info, *req_fh, **fh_kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/hrit_jma.py", line 278, in __init__
    self.area = self._get_area_def()
                ^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/hrit_jma.py", line 365, in _get_area_def
    "loff": self._get_line_offset(),
            ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Work/meteo/venv/lib/python3.12/site-packages/satpy/readers/hrit_jma.py", line 349, in _get_line_offset
    loff -= (self.mda["total_no_image_segm"] - segment_number - 1) * nlines
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
OverflowError: Python integer 550 out of bounds for uint8

@mraspaud
Copy link
Member

mraspaud commented Oct 15, 2024

@k3a thanks for your contribution! Do you think you can add a little test for this (to ensure that the bug doesn't come back in the future), and add your name to the Authors files?

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.08%. Comparing base (d6b8d01) to head (f36a096).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2930      +/-   ##
==========================================
- Coverage   96.09%   96.08%   -0.02%     
==========================================
  Files         377      377              
  Lines       55125    55125              
==========================================
- Hits        52975    52967       -8     
- Misses       2150     2158       +8     
Flag Coverage Δ
behaviourtests 3.94% <0.00%> (ø)
unittests 96.18% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Oct 17, 2024

Pull Request Test Coverage Report for Build 11632483694

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.191%

Totals Coverage Status
Change from base Build 11628253798: 0.0%
Covered Lines: 53211
Relevant Lines: 55318

💛 - Coveralls

@k3a
Copy link
Contributor Author

k3a commented Oct 19, 2024

No new tests seem necessary. I have just updated the _get_mda helper function to convert into the proper numpy types according to the dtype specs.

@djhoese
Copy link
Member

djhoese commented Oct 19, 2024

If the existing tests didn't fail before your fix then we need new tests that would fail and now pass unless I'm misunderstanding something.

@pnuu
Copy link
Member

pnuu commented Oct 19, 2024

I don't have an up-to-date environment at hand, but I think the modified data represents more closely the actual data, so might fail without the changes in the code. I'll see if I have the time to create a new env to test with.

@k3a
Copy link
Contributor Author

k3a commented Oct 19, 2024

I have, of course, tested that the current tests will fail without the fix. There are tests testing the case with segmented hrit but that _get_mda needs to retype args into proper np types to trigger this bug because when data is parsed it is parsed into those concrete types, not a generic python ints:

FAILED test_ahi_hrit.py::TestHRITJMAFileHandler::test_get_area_def - OverflowError: Python integer 275 out of bounds for uint8
FAILED test_ahi_hrit.py::TestHRITJMAFileHandler::test_get_dataset - OverflowError: Python integer 275 out of bounds for uint8
FAILED test_ahi_hrit.py::TestHRITJMAFileHandler::test_init - OverflowError: Python integer 11000 out of bounds for uint8
FAILED test_ahi_hrit.py::TestHRITJMAFileHandler::test_mask_space - OverflowError: Python integer 275 out of bounds for uint8

@pnuu
Copy link
Member

pnuu commented Nov 1, 2024

Closes #2963

@pnuu
Copy link
Member

pnuu commented Nov 1, 2024

Could you merge the Pytroll main to fix the failing tests?

And maybe next time create the PR from a new branch, I can't seem to be doing the update to your main.

Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@pnuu pnuu merged commit 8ff966e into pytroll:main Nov 1, 2024
16 of 18 checks passed
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.

5 participants