-
Notifications
You must be signed in to change notification settings - Fork 160
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
HDF5 OPERA IMPORTER FAILS READING QUALITY DATA #172
HDF5 OPERA IMPORTER FAILS READING QUALITY DATA #172
Conversation
Codecov Report
@@ Coverage Diff @@
## master #172 +/- ##
=========================================
Coverage ? 74.05%
=========================================
Files ? 106
Lines ? 8718
Branches ? 0
=========================================
Hits ? 6456
Misses ? 2262
Partials ? 0
Continue to review full report at Codecov.
|
Hi @chiara-arpae, welcome to pysteps! Thanks a lot for the PR. I'll take a look at it shortly. Are the DPC's HDF files available online? They would be helpful to test the code and to compare them with the other OPERA files we have in the pysteps-data repo that are being used to test the library. |
Hi @chiara-arpae, thanks a lot for the PR!! This week unfortunately I won't be able to look into it, but I see that @aperezhortal is already on it, (thanks!). Perhaps also @pulkkins would like to have a look, since he wrote the opera importer in the first place? We might want to add a test for reading the quality data, as I don't think we currently test for them. |
Hi!
DPC's HDF files aren't available online.
Anyway I got the permission to share with you a little dataset, relative to
entire 4/6/2020 and 5/6/2020 00:00 up to 5/6/2020 11:00 UTC.
You can access them on directories:
https://drive.google.com/drive/folders/1lkptGy1C2g48qSphowlnCq_czJ-97k6M?usp=sharing
https://drive.google.com/drive/folders/1Nt-abSl_zVr9Lim7hj_dfNNS5yS3dtGL?usp=sharing
Let me know if there are any problems in accessing these files.
Hoping these would be helpful.
Chiara
Il giorno mar 1 set 2020 alle ore 17:03 Andres Perez Hortal <
notifications@github.com> ha scritto:
… Hi @chiara-arpae <https://github.com/chiara-arpae>, welcome to pysteps!
Thanks a lot for the PR.
I'll take a look at it shortly. Are the DPC's HDF files available online?
They would be helpful to test the code and to compare them with the other
OPERA files we have in the pysteps-data repo that are being used to test
the library.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGRNPYKDD5IJ3AJVUK3SDUEMBANCNFSM4QRXQ5GQ>
.
|
@chiara-arpae That is more enough. Thanks! The modifications work well with the DPC and the OPERA files that we have in the pysteps-data repo (PAAH opera product I think). As you pointed out, the files in the pysteps-data repo organize the data in a slightly different way. Below there is an overview of the context of each opera files for reference (blue square: the PAAH opera product in the pysteps-data repo, below in red: the files from the DPC). Right now, the importer looks for the quality data in two places: inside the "data" group, and in a separate dataset. However, if the quality data is found in both places, it returns the quality data from the separate dataset in the case this one is placed after the "precipitation" dataset. I am wondering if we should leave this as the default behavior, or add a keyword to control which quality flag is prioritized? Although this is not strictly a problem for the PAAH and the DPC files, I don't know if there are other Opera files using a different data layout. What do you think @chiara-arpae, @pulkkins, @dnerini? |
Hi!
I think your proposal of introducing a keyword to optimize the search of
the quality data is a good idea, even if I imagine it would not add a great
improvement to the present modification of the code in terms of
generalization.
Actually I can't find a way to make the implementation of 'opera importer'
more general than the current one, in order to provide compatibility with
any eventual different OPERA file in importing its data.
Moreover incompatibility could not occur, we can't say.
Waiting for your decisions and news,
Good weekend!
Chiara
Il giorno gio 3 set 2020 alle ore 20:22 Andres Perez Hortal <
notifications@github.com> ha scritto:
… @chiara-arpae <https://github.com/chiara-arpae> That is more enough.
Thanks!
The modifications work well with the DPC and the OPERA files that we have
in the pysteps-data repo (PAAH opera product I think). As you pointed out,
the files in the pysteps-data repo organize the data in a slightly
different way.
Below there is an overview of the context of each opera files for
reference (blue square: the PAAH opera product in the pysteps-data repo,
below in red: the files from the DPC).
[image: image]
<https://user-images.githubusercontent.com/16256571/92150230-1b57ea80-eded-11ea-8a95-82ef8f2061c0.png>
Right now, the importer looks for the quality data in two places: inside
the "data" group, and in a separate dataset. However, if the quality data
is found in both places, it returns the quality data from the separate
dataset in the case this one is placed after the "precipitation" dataset. I
am wondering if we should leave this as the default behavior, or add a
keyword to control which quality flag is prioritized? Although this is not
strictly a problem for the PAAH and the DPC files, I don't know if there
are other Opera files using a different data layout. What do you think
@chiara-arpae <https://github.com/chiara-arpae>, @pulkkins
<https://github.com/pulkkins>, @dnerini <https://github.com/dnerini>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGV4COU7FW6BANFWXZLSD7NHJANCNFSM4QRXQ5GQ>
.
|
In pysteps, so far, there was no real ambition to provide general data importers: each importer was implemented to read a specific radar product and format only. The existing importers are simply a consequence of the datasets that are used by pysteps contributors (e.g., I work with MeteoSwiss radar composites and therefore implemented the importers for those data). If we continue to follow this same logic, the Please note that this is only true if the contribution by @chiara-arpae is a consequence of a legit difference between the OPERA and DPC datasets, and not because of a bug in the Finally, I would be in favor of implementing a more general importer for datasets following the ODIM specifications, i.e. a To sum up:
@chiara-arpae what do you think? @aperezhortal @pulkkins any thoughts? |
I think including a new importer according to ODIM specifications would be
great and very helpful.
Thank you very much for your kind attention.
Good day,
Chiara
Il giorno dom 6 set 2020 alle ore 14:32 Daniele Nerini <
notifications@github.com> ha scritto:
… In pysteps, so far, there was no real ambition to provide general data
importers: each importer was implemented to read a specific radar product
and format only. The existing importers are simply a consequence of the
datasets that are used by pysteps contributors (e.g., I work with
MeteoSwiss radar composites and therefore implemented the importers for
those data).
If we continue to follow this same logic, the import_opera_hdf5 importer
needs to be able to read OPERA composites (i.e. the pan-european composite)
in the HDF5 format only, while the DPC HDF5 files, which use a slightly
different structure, would be using a separate importer called
import_dpc_hdf5 or even import_arpae_hdf5, for example. This means that
this PR should not change the existing ìmport_opera_hdf5`, but instead
implement a new one.
Please note that this is only true if the contribution by @chiara-arpae
<https://github.com/chiara-arpae> is a consequence of a legit difference
between the OPERA and DPC datasets, and not because of a bug in the
import_opera_hdf5 function.
Finally, I would be in favor of implementing a more general importer for
datasets following the ODIM specifications
<https://www.eumetnet.eu/wp-content/uploads/2019/01/ODIM_H5_v23.pdf>,
i.e. a import_odim_hdf5 function.
To sum up:
- if there is nothing wrong with the current import_opera_hdf5
importer, I suggest not modifying it (again, here we consider that "opera"
refers to the pan-european composite product only, while "odim" is the data
model used by opera).
- instead, we can either include a new import_dpc_hdf5 importer or a
more general import_odim_hdf5 one, if this proves to be possible.
@chiara-arpae <https://github.com/chiara-arpae> what do you think?
@aperezhortal <https://github.com/aperezhortal> @pulkkins
<https://github.com/pulkkins> any thoughts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGXESV7BFCBQQC2YRTLSEN6MZANCNFSM4QRXQ5GQ>
.
|
Good, thanks for the feedback, @chiara-arpae. |
Ok for the refactoring.
I can write you when I'll load the changes.
Thanks,
Chiara
Il giorno mer 9 set 2020 alle ore 07:23 Daniele Nerini <
notifications@github.com> ha scritto:
… Good, thanks for the feedback, @chiara-arpae
<https://github.com/chiara-arpae>.
What you suggest for this PR? Do you want to refactor you changes into the
new ODIM importer?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGT2XS5PZ3U2KRND3PDSE4GNDANCNFSM4QRXQ5GQ>
.
|
My suggestion is to replace the old importer (import_opera_hdf5) with the new one and rename it as import_odim_hdf5 (and modify the docstring accordingly). When I wrote import_opera_hdf5, I tried to make it as general as possible and not to include anything that is specific to the HDF5 files provided by OPERA. I totally agree that someone needs to implement an importer that fully supports the ODIM specification. Now it only supports a small subset of it. So, the restrictions should be clearly described in the docstring if we decide to rename it as import_odim_hdf5. The main restriction is that it currently reads only one dataset from the file. |
Hi!
Thank-you very much for your suggestion.
Actually, I was about to push the modifies, consisting of one only
additional importer hdf5_opera, suitable for reading DPC hdf5 files and
separated from the existing one.
So, if you prefer, I can wait and make other modifies in order to replace
the old opera_importer with a new one, searching for quality data in
different places for example according to the filename that contains
information about the institution and using the quantity attribute
information.
Otherwise, If you prefer providing a generalization of the code by your
own, I thank you for your availability and attention to this PR anyway.
Waiting for news,
Thank you all.
Chiara
Il giorno ven 11 set 2020 alle ore 14:58 Seppo Pulkkinen <
notifications@github.com> ha scritto:
… In pysteps, so far, there was no real ambition to provide general data
importers: each importer was implemented to read a specific radar product
and format only. The existing importers are simply a consequence of the
datasets that are used by pysteps contributors (e.g., I work with
MeteoSwiss radar composites and therefore implemented the importers for
those data).
If we continue to follow this same logic, the import_opera_hdf5 importer
needs to be able to read OPERA composites (i.e. the pan-european composite)
in the HDF5 format only, while the DPC HDF5 files, which use a slightly
different structure, would be using a separate importer called
import_dpc_hdf5 or even import_arpae_hdf5, for example. This means that
this PR should not change the existing ìmport_opera_hdf5`, but instead
implement a new one.
Please note that this is only true if the contribution by @chiara-arpae
<https://github.com/chiara-arpae> is a consequence of a legit difference
between the OPERA and DPC datasets, and not because of a bug in the
import_opera_hdf5 function.
Finally, I would be in favor of implementing a more general importer for
datasets following the ODIM specifications
<https://www.eumetnet.eu/wp-content/uploads/2019/01/ODIM_H5_v23.pdf>,
i.e. a import_odim_hdf5 function.
To sum up:
* if there is nothing wrong with the current `import_opera_hdf5` importer, I suggest not modifying it (again, here we consider that "opera" refers to the pan-european composite product only, while "odim" is the data model used by opera).
* instead, we can either include a new `import_dpc_hdf5` importer or a more general `import_odim_hdf5` one, if this proves to be possible.
@chiara-arpae <https://github.com/chiara-arpae> what do you think?
@aperezhortal <https://github.com/aperezhortal> @pulkkins
<https://github.com/pulkkins> any thoughts?
In pysteps, so far, there was no real ambition to provide general data
importers: each importer was implemented to read a specific radar product
and format only. The existing importers are simply a consequence of the
datasets that are used by pysteps contributors (e.g., I work with
MeteoSwiss radar composites and therefore implemented the importers for
those data).
If we continue to follow this same logic, the import_opera_hdf5 importer
needs to be able to read OPERA composites (i.e. the pan-european composite)
in the HDF5 format only, while the DPC HDF5 files, which use a slightly
different structure, would be using a separate importer called
import_dpc_hdf5 or even import_arpae_hdf5, for example. This means that
this PR should not change the existing ìmport_opera_hdf5`, but instead
implement a new one.
Please note that this is only true if the contribution by @chiara-arpae
<https://github.com/chiara-arpae> is a consequence of a legit difference
between the OPERA and DPC datasets, and not because of a bug in the
import_opera_hdf5 function.
Finally, I would be in favor of implementing a more general importer for
datasets following the ODIM specifications
<https://www.eumetnet.eu/wp-content/uploads/2019/01/ODIM_H5_v23.pdf>,
i.e. a import_odim_hdf5 function.
To sum up:
* if there is nothing wrong with the current `import_opera_hdf5` importer, I suggest not modifying it (again, here we consider that "opera" refers to the pan-european composite product only, while "odim" is the data model used by opera).
* instead, we can either include a new `import_dpc_hdf5` importer or a more general `import_odim_hdf5` one, if this proves to be possible.
@chiara-arpae <https://github.com/chiara-arpae> what do you think?
@aperezhortal <https://github.com/aperezhortal> @pulkkins
<https://github.com/pulkkins> any thoughts?
My suggestion is to replace the old importer (import_opera_hdf5) with the
new one and rename it as import_odim_hdf5 (and modify the docstring
accordingly). When I wrote import_opera_hdf5, I tried to make it as general
as possible and not to include anything that is specific to the HDF5 files
provided by OPERA.
I totally agree that someone needs to implement an importer that fully
supports the ODIM specification. Now it only supports a small subset of it.
So, the restrictions should be clearly described in the docstring if we
decide to rename it as import_odim_hdf5. The main restriction is that it
currently reads only one dataset from the file.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGXUBQ7EZ7MCJLDWRJDSFING7ANCNFSM4QRXQ5GQ>
.
|
Along these lines, to maintain backward compatibility, we should leave the import_opera_hdf5 function for a while, and add also in parallel a general odim importer. Since the pan-european composite is a particular case of the ODIM standard, the general odim imported could have keywords that control which particular type of product is read (pan-european, DPC, generic, etc.). @chiara-arpae, do you want to push the changes? We can use that implementation as a starting point for the more general implementation. I have the impression that a more general ODIM implementation will take a considerable effort and it may be outside the scope of this PR. Perhaps we can start small, merging this PR with an odim reader that deals with the pan-european and DPC format, and then, incrementally make it more general. |
Push done.
Hoping it would be useful,
thank you for your kind attention,
Chiara
Il giorno ven 11 set 2020 alle ore 15:56 Andres Perez Hortal <
notifications@github.com> ha scritto:
… To sum up:
- if there is nothing wrong with the current import_opera_hdf5
importer, I suggest not modifying it (again, here we consider that "opera"
refers to the pan-european composite product only, while "odim" is the data
model used by opera).
- instead, we can either include a new import_dpc_hdf5 importer or a
more general import_odim_hdf5 one, if this proves to be possible.
Along these lines, to maintain backward compatibility, we should leave the
import_opera_hdf5 function for a while, and add also in parallel a general
odim importer. Since the pan-european composite is a particular case of the
ODIM standard, the general odim imported could have keywords that control
which particular type of product is read (pan-european, DPC, generic, etc.).
@chiara-arpae <https://github.com/chiara-arpae>, do you want to push the
changes? We can use that implementation as a starting point for the more
general implementation. I have the impression that a more general ODIM
implementation will take a considerable effort and it may be outside the
scope of this PR.
Perhaps we can start small, merging this PR with an odim reader that deals
with the pan-european and DPC format, and then, incrementally make it more
general.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGXWAVXXLAX4AQAIXH3SFIUCPANCNFSM4QRXQ5GQ>
.
|
My apologies for this lengthy process, unfortunately I had little time to dedicate to pysteps lately. Can we all try to quickly summarize the point at which we are now? Also, I believe @aperezhortal had an idea on how to proceed with this PR? From what I gathered so far, we want to
Agreed ? |
Hi @chiara-arpae, sorry for stalling on this. I agree with @dnerini comments. The original goal of this PR was to fix the problem with the opera importer reading the quality data from the DPC files. Looking backward, commit b5aa2fe solved that issue and Although we all agreed that a general ODIM importer is a nice addition to pysteps, I think that it is better to maintain it outside the scope of this PR. Following on @Daniele comments, a possible way to proceed with this PR is:
The following steps can address this point:
This is addressed by adding After this PR is merged, we can incrementally work on the more general ODIM reader by adding functionalities to What do you think? |
This reverts commit 80a8919 and reverts the works to commit b5aa2fe as requested by @aperezhortal in pySTEPS#172 (comment)
Hi you all,
thank you for writing about the PR and sorry for late reply.
I agree with you.
I've just pushed new changes, following your advice about making a wrapper
and renaming the 'import_opera_hdf5' to 'import_odim_hdf5'.
For this, I'm very grateful to the computer scientist team of Arpae, who
made these changes.
I only add one more code line at
f9265ac
#f926ac in order to search for quality field in the most underlaying level
of the hierarchy only if it results None after the reading part that works
for PAAH hdf files but not for DPC files. Otherwise, if quality is not None
- that is the case of PAAH hdf files - the quality array stored will not be
overwritten.
It could be a fast and maybe nice way to avoid keywords.
What do you think?
Thank you very much for your attention,
Chiara
Il giorno sab 3 ott 2020 alle ore 21:17 Andres Perez Hortal <
notifications@github.com> ha scritto:
… Hi @chiara-arpae <https://github.com/chiara-arpae>, sorry for stalling on
this.
I agree with @dnerini <https://github.com/dnerini> comments.
The original goal of this PR was to fix the problem with the opera
importer reading the quality data from the DPC files. Looking backward,
commit b5aa2fe
<b5aa2fe>
solved that issue and import_opera_hdf5 deals with the OPERA and DCP
files.
Although we all agreed that a general ODIM importer is a nice addition to
pysteps, I think that it is better to maintain it outside the scope of this
PR.
Following on @Daniele <https://github.com/Daniele> comments, a possible
way to proceed with this PR is:
implement first version of general ODIM importer named import_odim_hdf5,
which can deal with both OPERA and DCP.
The following steps can address this point:
1. Revert to commit b5aa2fe
<b5aa2fe>
2. Rename import_opera_hdf5 to import_odim_hdf5
3. Update the doc-strings to mention that the OPERA and the DPC files
are correctly supported at the moment. Also make clear that other
ODIM-compliant files may not me read correctly.
we keep the old import_opera_hdf5 for backward compatibility
This is addressed by adding import_opera_hdf5=import_odim_hdf5 in the
importers module .
After this PR is merged, we can incrementally work on the more general
ODIM reader by adding functionalities to import_odim_hdf5.
What do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ2PKGSREZ4KCYDY67AAQELSI52DNANCNFSM4QRXQ5GQ>
.
|
Thanks @chiara-arpae! I left two additional suggestions. Besides that, it looks ready to merge for me. |
Hi @chiara-arpae (and colleagues), thanks a lot for the nice contribution. This looks good to be merged! |
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last detail, we forgot to update the information in io/interface.py
. This will only take few more lines:
- [ ] add the new importer to the _importer_methods
dictionary: odim_hdf5=importers.import_odim_hdf5
- [ ] update table in the get_method
docstrings.
Edit: Well, I suppose this is something I can myself after merging, too ;-)
Hi!
I changed the code of the implementation file for pysteps importers in the data importer from OPERA HDF5 files, 'opera_hdf5'.
Actually the path where quality dataset is placed in OPERA HDF5 files by DPC didn't correspond to the path used in the previous code.
Consequently quality data couldn't be read and a list of None, long as the number of timesteps wanted to read, was returned instead of the expected nd-array of shape (t,x,y), where t is the number of timesteps, x and y the grid points coordinates of the domain.
Before the quality dataset was searched looking if the quantity defined in the group attributes was 'QIND', but looking in a higher level of the hierarchy. Instead quality data are in the dataset inside a group in the underlying level respect to the group containing rainrate data.
In this way, I search for quality data, looking for the group containing 'quality' in its name and then it is verified it contains 'QIND' as the quantity attribute.
I hope you appreciate the issue and receive your ouput soon.
Fix #171