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 MIRS reader #1486

Closed
wants to merge 6,859 commits into from
Closed

Add MIRS reader #1486

wants to merge 6,859 commits into from

Conversation

joleenf
Copy link
Contributor

@joleenf joleenf commented Dec 15, 2020

  • Work in Progress PR takes over for [WIP] Add MIRS file reader #1285.
  • Minor adjustments have been made to add coordinates, rename the dimensions and update code for retrieving sensor and platform names.
  • Brightness Temperature dataset ID's are available in dataset ID list. Started separating get_dataset call into data that can be loaded without modification and the 3D sets that need work. The 3D section is not complete and not correct since no limb correction is being applied.
  • Tests remain the same as Katherine Kolman's work at this time.
  • Passes flake8 satpy

Nina.Hakansson and others added 30 commits December 1, 2020 15:29
Includes hack for hanging test that is yet to be solved
Add support for s3 buckets in OLCI and ABI l1 readers
Removes alteration on ABI BT enhancements
1.)  Multiple changes to support use of dask.array.map_blocks rather
than memmap for limb correction
2.)  BUG FIX to selection of data based on DataID
3.)  Added test code
Merge branch 'mirs_xarray' of github.com:joleenf/satpy into mirs_reader
1.)  Multiple changes to support use of dask.array.map_blocks rather
than memmap for limb correction
2.)  BUG FIX to selection of data based on DataID
3.)  Added test code
Merge branch 'mirs_xarray' of github.com:joleenf/satpy into mirs_reader
etc/readers/mirs_l2_nc.yaml
1.)  use [longitude, latitude] for consistency with mirs reader code
2.)  add long_name and use name to be same as file_key because two
     DataIDs were getting set and only one was working.  (what now
     is long_name was not working as name)
readers/mirs_l2_nc.py
1.)  Change variables name of "Latitude" and "Longitude"
    to "latitude" and "longitude" so that coordinate variables
    are longitude and latitude, because the mixed case version
    was not getting consitently recognized as coordinates...and
    it could be the way I was defining the coordinate variables
    at the time.
2.)  Make sure coordinate attributes are added when the available
     datasets are built.  This helps later when the SwathDefinition
     automatically is created.
3.) Add a CRS in data attributes for xrImage.  Use WGS84, CRS(4326),
    consistent with the SwathDefinition default.
tests/reader_tests/test_mirs_l2_nc.py
Don't make test arrays so complicated.
etc/readers/mirs_l2_nc.yaml
1.)  use [longitude, latitude] for consistency with mirs reader code
2.)  add long_name and use name to be same as file_key because two
     DataIDs were getting set and only one was working.  (what now
     is long_name was not working as name)
readers/mirs_l2_nc.py
1.)  Change variables name of "Latitude" and "Longitude"
    to "latitude" and "longitude" so that coordinate variables
    are longitude and latitude, because the mixed case version
    was not getting consitently recognized as coordinates...and
    it could be the way I was defining the coordinate variables
    at the time.
2.)  Make sure coordinate attributes are added when the available
     datasets are built.  This helps later when the SwathDefinition
     automatically is created.
3.) Add a CRS in data attributes for xrImage.  Use WGS84, CRS(4326),
    consistent with the SwathDefinition default.
tests/reader_tests/test_mirs_l2_nc.py
Don't make test arrays so complicated.
… BT.

1.)  Single dimension coordinates like Freq cannot be mapped in raw form.
2.)  A little work needs to be done to make 3D vars other than BT displayable.
     It is possible, but also adds another layer to the code.  Since they do
     not need to be supported don't add for now.
3.)  Make sure code that skips limb correction for the short term does not
     introduce a bug when uncommented or limb correction is true. (Therefore,
     the changes to where bt_data is reduced to 2D and the do_not_apply
     boolean.
… BT.

1.)  Single dimension coordinates like Freq cannot be mapped in raw form.
2.)  A little work needs to be done to make 3D vars other than BT displayable.
     It is possible, but also adds another layer to the code.  Since they do
     not need to be supported don't add for now.
3.)  Make sure code that skips limb correction for the short term does not
     introduce a bug when uncommented or limb correction is true. (Therefore,
     the changes to where bt_data is reduced to 2D and the do_not_apply
     boolean.
This was causing issues because one of the coordinates was being found twice
for DataIds and that was messy.  Just take it out, the coordinates should
be found in the reader code.
This was causing issues because one of the coordinates was being found twice
for DataIds and that was messy.  Just take it out, the coordinates should
be found in the reader code.
Previous to this, the date and time were split causing find_files_and_readers
to interpret the datetime of the file as datetime(1900,1,1,16,1) and that
is not correct.  This would cause the reader to miss files when start_time/end_time
combinations were used for metop files.  This commit parses the date+"strings in between"+
time as one so that the start_time can be interpreted properly as something like
dateime(2017,2,6,16,1) <== that being the example from the test file...
IMG_SX.M2.D17037.S1601.E1607.B0000001.WE.HR.ORB.nc
Previous to this, the date and time were split causing find_files_and_readers
to interpret the datetime of the file as datetime(1900,1,1,16,1) and that
is not correct.  This would cause the reader to miss files when start_time/end_time
combinations were used for metop files.  This commit parses the date+"strings in between"+
time as one so that the start_time can be interpreted properly as something like
dateime(2017,2,6,16,1) <== that being the example from the test file...
IMG_SX.M2.D17037.S1601.E1607.B0000001.WE.HR.ORB.nc
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

A couple quick questions/changes. Hopefully can answer your questions from slack after a little more discussion.

from pyproj import CRS
from satpy.readers.file_handlers import BaseFileHandler

LOAD_CHUNK_SIZE = int(os.getenv('PYTROLL_LOAD_CHUNK_SIZE', -1))
Copy link
Member

Choose a reason for hiding this comment

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

This should be taken from satpy import CHUNK_SIZE so it can be updated more easily in the future. The environment variable is PYTROLL_CHUNK_SIZE that controls that value.


return new_ds

@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

There are some cases (multi-process scheduler) where dask doesn't always work well with functions that aren't globally defined. I'm not sure if staticmethods have this problem, but what are your thoughts on moving this staticmethod (and any other delayed/map_blocks'd methods) to be functions in the global scope?

1.)  CHUNK_SIZE is used to make future updates easier
2.)  Move dask operations out of staticmethod to a global method
Reason:  There are cases (multi-process scheduler) when dask
doesn't work well with functions that aren't globally defined.
3.)  BUG FIX:  noaa-20 should be atms sensor, so that limb correction
is applied as promised.
1.)  CHUNK_SIZE is used to make future updates easier
2.)  Move dask operations out of staticmethod to a global method
Reason:  There are cases (multi-process scheduler) when dask
doesn't work well with functions that aren't globally defined.
3.)  BUG FIX:  noaa-20 should be atms sensor, so that limb correction
is applied as promised.
filter-branch
Merge branch 'mirs_reader' of github.com:joleenf/satpy into mirs_reader
@joleenf joleenf marked this pull request as ready for review January 21, 2021 18:28
@joleenf
Copy link
Contributor Author

joleenf commented Jan 21, 2021

Hi @djhoese, I have moved the limb_correction code to a global function. CHUNK_SIZE is being used. I believe I have removed all of the correction files that I have inadvertently committed. The correction is currently not active due to the fact that we need to figure out where those correction files will be stored.

@joleenf
Copy link
Contributor Author

joleenf commented Jan 21, 2021

@djhoese I probably should have left this as a draft considering the question of coeffcient files.

@djhoese
Copy link
Member

djhoese commented Jan 21, 2021

Hm it seems you have confused github (6,859 commits). Yay git! It looks like the commands you ran to remove the files from your commits must have gone through and edited almost all Satpy commits (master has ~8k commits). If you can't figure it out, it may be worth copying your changed files to a new branch and making a new PR. Sorry.

@djhoese
Copy link
Member

djhoese commented Jan 21, 2021

Additionally it looks like one of the test environments failed.

@joleenf
Copy link
Contributor Author

joleenf commented Jan 21, 2021

Oh. No.

@joleenf joleenf closed this Jan 21, 2021
joleenf added a commit to joleenf/satpy that referenced this pull request Jan 22, 2021
@joleenf joleenf mentioned this pull request Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants