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 fci_l1c_fdhsi to fci_l1c_nc #1684

Closed
ameraner opened this issue May 20, 2021 · 6 comments · Fixed by #1712
Closed

Rename fci_l1c_fdhsi to fci_l1c_nc #1684

ameraner opened this issue May 20, 2021 · 6 comments · Fixed by #1712
Assignees
Labels
component:readers documentation enhancement code enhancements, features, improvements PCW Pytroll Contributors' Week

Comments

@ameraner
Copy link
Member

Feature Request

Is your feature request related to a problem? Please describe.
The current FCI L1c reader is called fci_l1c_fdhsi, where fdhsi indicates the imaging mode (Full Disc High Spectral resolution Imagery). The counterpart of fdhsi is the hrfi imaging mode (High spatial Resolution Fast Imagery). HRFI images will be distributed in separate NetCDF4 files, that however have exactly the same filetype and structure as FDHSI files. The channels inside the HRFI, and therefore the Satpy datasets, will have different names (e.g. vis_06_hr in HRFI instead of vis_06 in FDHSI).

Keeping the current fci_l1c_fdhsi reader name implies that in future, when the first (test) data for HRFI will be available, we will have to implement an fci_l1c_hrfi reader.

Since the filetype and structure is essentially identical, there is however no reason to have two readers defined.

Describe the solution you'd like
Rename the fci_l1c_fdhsi to fci_l1c_nc.
With this

  • we have only one reader for all FCI L1c files, making the usage of Satpy simpler
  • we comply to the usual convention of having readers called as instrument_level_format
  • by having two filetypes in the yaml, we can still assign the _hr datasets to the HRFI files, and, if necessary, have different handling in the filehandlers.

This PR comes a little late, but fortunately not too late, as released test data has not spread too much yet, and limited users are already using Satpy for it.
However, there are already users/software that have implemented workflows using the old reader name, and some documentation.

I would therefore propose to rename the reader, but find a solution to keep the old name working for a while.

The best solution in my opinion would be to use the OLD_READERS mechanism in configs_for_reader

if reader_name.endswith('.yaml') or reader_name not in OLD_READER_NAMES:
new_readers.append(reader_name)
continue
new_name = OLD_READER_NAMES[reader_name]
# Satpy 0.11 only displays a warning
# Satpy 0.13 will raise an exception
raise ValueError("Reader name '{}' has been deprecated, use '{}' instead.".format(reader_name, new_name))
# Satpy 0.15 or 1.0, remove exception and mapping

in the current implementation, however, a hard ValueError is raised. To make the transition smoother, I would propose to go back to a Warning instead, at least for a while. This means basically undoing what @djhoese did in #597. Note that OLD_READERS is currently empty, so returning to a Warning would not affect any other workflow.

Another, less elegant, solution would be to keep the old yaml file around, pointing to the renamed FileHandler.

Please let me know your thoughts, if you agree with my preferred solution or if I'm missing something.

FYI @gerritholl @mraspaud @sjoro

@ameraner ameraner added enhancement code enhancements, features, improvements documentation component:readers PCW Pytroll Contributors' Week labels May 20, 2021
@ameraner ameraner self-assigned this May 20, 2021
@ameraner ameraner added this to Review in progress in PCW Spring 2021 May 20, 2021
@mraspaud
Copy link
Member

I support the renaming!

Regarding the Warning vs Error: We might need this in the future again, so how about having an extra PENDING_OLD_READER_NAMES also that would issue the warning? and then when the pending period is over, the reader name is moved to OLD_READER_NAMES ?

@sjoro
Copy link
Collaborator

sjoro commented May 21, 2021

the first prototype of the reader was created before trying to streamline and/or rationalise the names. so, i also support the renaming!

@gerritholl
Copy link
Collaborator

The first prototype of this reader was calledfci_fdhsi and was added in #13.

I am in doubt about the name change. The current name is consistent with the naming pattern <sensor>[_<processing level>[_<level detail>]][_<file format>]. I find it somewhat debatable whether NetCDF is a complete format identifier. The format identifier fdhsi is more precies than nc. There can be many file formats using NetCDF. What if some future project reprocesses the FCI record and publishes the files in a different file format that is also based on NetCDF? This reader doesn't just read any NetCDF file containing FCI data, it reads the official EUMETSAT format as defined in the PUG EUM/MTG/USR/13/719113. Should that be captured in the reader name?

@ameraner
Copy link
Member Author

@mraspaud I like the idea of the PENDING_OLD_READER_NAMES. Should it be a DeprecationWarning then?

@gerritholl In your scenario fdhsi would probably not be exhaustive either, since a potential reprocessing could be explicitly reprocessing fdhsi data as well, so we wouldn't solve the problem.
I would propose that the "official" data files (the ones being operationally disseminated by the producer) could be sensor_processinglevel_fileformat for the sake of simplicity. If a secondary (reprocessed) dataset is created, we can add a new name component, similarly to what we have now for e.g. mviri_l1b_fiduceo_nc. What do you think?

@mraspaud
Copy link
Member

yes, DeprecationWarning sounds good!

@ameraner
Copy link
Member Author

The PENDING_OLD_READER_NAMES is being added in #1684

PCW Spring 2021 automation moved this from Review in progress to Done Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers documentation enhancement code enhancements, features, improvements PCW Pytroll Contributors' Week
Projects
Development

Successfully merging a pull request may close this issue.

4 participants