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

Rename readers to meet new reader naming scheme #546

Merged
merged 35 commits into from
Dec 18, 2018

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Dec 12, 2018

For details on the discussion that lead to the changes in this PR see #527. The bottom line is that the names of readers in SatPy are fairly inconsistent. This PR is an attempt to finalize the naming convention and rename existing readers to match this scheme.

Readers Changed

  • avhrr_aapp_l1b -> avhrr_l1b_aapp
  • avhrr_eps_l1b -> avhrr_l1b_eps
  • avhrr_hrpt_l1b -> avhrr_l1b_hrpt
  • fci_fdhsi -> fci_l1c_fdhsi
  • gac_lac_l1 -> avhrr_l1b_gaclac
  • ghrsst_osisaf -> ghrsst_l3c_sst
  • hdf4_caliopv3 -> caliop_l2_cloud
  • hdfeos_l1b -> modis_l1b
  • hrit_electrol -> electrol_hrit
  • hrit_goes -> goes-imager_hrit
  • hrit_jma -> ahi_hrit
  • hrit_msg -> seviri_l1b_hrit
  • native_msg -> seviri_l1b_native
  • nc_goes -> goes-imager_nc
  • nc_nwcsaf_msg -> nwcsaf-geo
  • nc_nwcsaf_pps -> nwcsaf-pps_nc
  • nc_olci_l1b -> olci_l1b
  • nc_olci_l2 -> olci_l2
  • nc_seviri_l1b -> seviri_l1b_nc
  • nc_slstr -> slstr_l1b
  • safe_msi -> msi_safe
  • safe_sar_c -> sar-c_safe
  • scmi_abi_l1b -> abi_l1b_scmi

Remaining/Undecided

  • avhrr_aapp_l1b, avhrr_eps_l1b, avhrr_hrpt_l1b: Aren't the middle part of these names technically the processing software? Or could they maybe be considered the "file format"?
  • gac_lac_l1: This looks like a AVHRR reader but I don't know what "gac_lac" is.
  • ghrsst_osisaf: It is originally just for AVHRR, but now supports AVHRR and VIIRS. Also OSISAF is technically the organization and not the processing software, right? Would osisaf-ghrsst_l2_sst be too annoying?
  • grib, technically this should probably be generic_grib but grib is already a generic format. If this stays grib then should generic_image become image? I'm considering this a non-issue right now
  • nc_nwcsaf_msg: Right now this only support seviri but @adybbroe said that the processing software actually supports other geostationary satellites and is now called NWCSAF Geo (or something). So possible names are: nwcsafgeo or nwcsaf_geo or nwcsaf-geo or nwcsaf-geo_nc.
  • nc_nwcsaf_pps: Same as above. PPS is Polar Platform System
  • nc_seviri_l1b: Is this the "official" product for level 1b? If so then this could be called seviri_l1b. If not, then seviri_l1b_nc.

@djhoese djhoese added enhancement code enhancements, features, improvements component:readers refactor backwards-incompatibility Causes backwards incompatibility or introduces a deprecation labels Dec 12, 2018
@djhoese djhoese added this to the v0.11 milestone Dec 12, 2018
@djhoese djhoese self-assigned this Dec 12, 2018
@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #546 into master will increase coverage by 0.26%.
The diff coverage is 98.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   76.52%   76.78%   +0.26%     
==========================================
  Files         136      136              
  Lines       18908    19000      +92     
==========================================
+ Hits        14469    14590     +121     
+ Misses       4439     4410      -29
Impacted Files Coverage Δ
satpy/readers/fci_l1c_fdhsi.py 20.45% <ø> (ø)
satpy/readers/avhrr_l1b_gaclac.py 8.51% <ø> (ø)
satpy/tests/reader_tests/test_ahi_hrit.py 96.58% <ø> (ø)
satpy/tests/reader_tests/test_viirs_edr_flood.py 94.59% <ø> (ø)
satpy/readers/seviri_base.py 97.1% <ø> (ø)
satpy/readers/goes_imager_hrit.py 51.65% <ø> (ø)
satpy/readers/electrol_hrit.py 34.14% <ø> (ø)
satpy/readers/ghrsst_l3c_sst.py 23.88% <ø> (ø)
satpy/readers/olci_nc.py 42.85% <ø> (ø)
satpy/readers/msi_safe.py 2.52% <ø> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6c92ed...eb8a5bd. Read the comment docs.

@djhoese
Copy link
Member Author

djhoese commented Dec 13, 2018

As mentioned on slack, @sjoro said that nc_seviri_l1b should be renamed to seviri_l1b_nc since the NetCDF product is not the "official" product (HRIT is, followed by native).

@coveralls
Copy link

coveralls commented Dec 14, 2018

Coverage Status

Coverage increased (+0.3%) to 76.789% when pulling eb8a5bd on djhoese:refactor-rename-readers into e6c92ed on pytroll:master.

@djhoese
Copy link
Member Author

djhoese commented Dec 16, 2018

@adybbroe I renamed nc_nwcsaf_msg/pps to nwcgeo/nwcpps based on the pages here:

Which use those names for the products. I didn't here any opinions on better names for this but would also be ok with something like nwcsaf-geo. Maybe?

Only reader remaining now is ghrsst_osisaf. I can't find any files online that match the file patterns for this reader so I don't know what the differences are between this reader's file and others and what a good name would be.

@sjoro @ColinDuff @mraspaud @pnuu Please review the names I've changed so far. I'm ok merging this as is and leaving ghrsst_osisaf if needed.

@sjoro
Copy link
Collaborator

sjoro commented Dec 17, 2018

SEVIRI and FCI readers look good, ok to merge from my side.

@mraspaud
Copy link
Member

We probably have to update a bunch of pytroll-examples notebooks in conjunction with this, right ?

@mraspaud
Copy link
Member

ghrsst_osisaf: It is originally just for AVHRR, but now supports AVHRR and VIIRS. Also OSISAF is technically the organization and not the processing software, right? Would osisaf-ghrsst_l2_sst be too annoying?

How about osisaf_l2_ghrsst (ghrsst is a product I think)

@mraspaud
Copy link
Member

The imager of Electro-L N°2 is MSU-GS, so it should be hrit_electrol -> msu-gs_hrit right ?

@mraspaud
Copy link
Member

Also, why seviri_l1b_hrit vs ahi_hrit ?
As for the NWCSAF products, I would rather have nwcsaf-geo and nwcsaf-pps_nc (there is a h5 version for older versions of the PPS software)

@ColinDuff
Copy link
Contributor

seviri name changes look good to me

@djhoese
Copy link
Member Author

djhoese commented Dec 17, 2018

@mraspaud:

  1. Updating examples: Yes, but since this PR still allows for the old names to be used we should be fine from the immediate "I tried this example and it didn't work".
  2. For ghrsst_osisaf, this page suggests level 3C. I took "osisaf" to be the producing/processing organization but my google searches made it seem like their format may be custom compared to others who produce the same product. Additionally the way it is used in the pages I've found is that "ghrsst" is the group that produces (or at least approved of) the algorithm. Maybe ghrsst_l2 or ghrsst_l3c then?
  3. Electro-L: Yes you are right for that specific instrument. When I was looking for it on OSCAR I found that every other platform uses "Electro-L N" for the name. See:
  1. For seviri_l1b_hrit, the l1b was added to be consistent with the other SEVIRI L1B reader's after talking with @sjoro. The ahi_hrit, as far as I know, is the only HRIT product released as hrit for AHI. I'd prefer not to add the l1b (is that even correct?) for AHI, but I'd be ok with removing l1b for the seviri hrit reader. I think I most prefer the way it is currently, but can be convinced otherwise.
  2. I'm ok with the nwcsaf-geo. For nwcsaf-pps, do you ever see us supporting the older format? Maybe it would be "cleaner" to have nwcsaf-pps_h5 reader if it is needed in the future?

@djhoese
Copy link
Member Author

djhoese commented Dec 17, 2018

@mraspaud Note I/we also need to update the directory names for the behavior tests since they currently use hrit_msg and other similar readers.

@mraspaud
Copy link
Member

  1. Very good
  2. Could we just omit the level while we wait for @adybbroe to give his opinion on that ?
  3. Ok
  4. Ok, we can leave it as it is.
  5. TBH, I'm generally not confortable with leaving the formats out, for the reason that what might be the regular format for some users might be different for others. In this particular case, the pps v2014 software can produce both nc and h5 files, both of which are officially supported. We chose to support nc, because h5 is going to be dropped in the next version, but h5 has been the historical format. So to avoid confusion I'd like to append _nc. The geo counterpart doesn't have this problem since the software name changed from nwcsaf_msg to nwcsaf_geo, but as per my preliminary comment on this point, I would prefer to have _nc appended. Yeah, you can call me pedantic :)

@djhoese
Copy link
Member Author

djhoese commented Dec 17, 2018

  1. So just ghrsst?
  2. I'm ok with adding the format. I think this is easily a 50/50 decision for me on this specific reader. I think most users coming to satpy who want to read the data are probably using the new format. If they are using the old format I would assume they already have non-satpy solutions for what they want to do. Let me know (here or slack) if you are ok with me not changing it. Otherwise, I'll make the changes tomorrow morning probably.

@mraspaud
Copy link
Member

  1. algorithm + product right ? so osisaf_ghrsst ?
  2. Yes, please add the format.

@djhoese
Copy link
Member Author

djhoese commented Dec 18, 2018

But isn't ghrsst the algorithm and the product? osisaf is the "processing center" if I'm understanding things, which our current naming scheme doesn't handle (nor do I want it to). For the naming scheme to use an "extra detail" field like "product" it has to include the level like viirs_edr_flood. I think if we are being strict about this naming, which we don't need to be, I'm fine with this being an exception to the rule, then I think it would be osisaf-ghrsst.

@mraspaud
Copy link
Member

ok, ghrsst it is.

@djhoese djhoese changed the title [WIP] Rename readers to meet new reader naming scheme Rename readers to meet new reader naming scheme Dec 18, 2018
@djhoese djhoese merged commit 4a4df8e into pytroll:master Dec 18, 2018
@djhoese djhoese deleted the refactor-rename-readers branch December 18, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatibility Causes backwards incompatibility or introduces a deprecation component:readers enhancement code enhancements, features, improvements refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SEP] Reader naming conventions
5 participants