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

Read wavelength ranges from netcdf #1544

Merged
merged 4 commits into from
Feb 15, 2021

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Feb 12, 2021

This PR should fix the reading of wavelengths from satpy-created netcdf files.

  • Tests added

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #1544 (0e9def4) into master (edcceb1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1544   +/-   ##
=======================================
  Coverage   92.08%   92.09%           
=======================================
  Files         251      251           
  Lines       36674    36711   +37     
=======================================
+ Hits        33771    33808   +37     
  Misses       2903     2903           
Flag Coverage Δ
behaviourtests 4.48% <14.28%> (+0.01%) ⬆️
unittests 92.62% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/dataset/dataid.py 94.05% <100.00%> (+0.33%) ⬆️
satpy/readers/satpy_cf_nc.py 98.59% <100.00%> (+0.02%) ⬆️
satpy/tests/reader_tests/test_satpy_cf_nc.py 100.00% <100.00%> (ø)
satpy/tests/test_dataset.py 100.00% <100.00%> (ø)
satpy/writers/cf_writer.py 93.99% <100.00%> (+0.11%) ⬆️

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 edcceb1...c52a07f. Read the comment docs.

@TAlonglong
Copy link
Collaborator

Thanks @mraspaud

The first commit with json seemed to work both writing and reading back over opendap from thredds.

The second one fails when reading back from thredds via opendap (https://thredds.met.no/thredds/dodsC/metusers/trygveas/platform_name-sensor-20200309121303-20200309122402.nc) But the wavelength seems ok.

  File "/home/trygveas/pytroll/satpy/satpy/dataset/dataid.py", line 103, in __eq__
    return other in self
  File "/home/trygveas/pytroll/satpy/satpy/dataset/dataid.py", line 137, in __contains__
    return self.min <= other <= self.max
TypeError: '<=' not supported between instances of 'str' and 'float'

Running in my debugger I see that other is float, and self.min and self.max are strings
I can not figure out where those are set.

@mraspaud
Copy link
Member Author

Yes, the error comes from trollsift, a new release is needed.

@TAlonglong
Copy link
Collaborator

OK, I saw you talked with David about trollsift.

@ghost
Copy link

ghost commented Feb 12, 2021

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

@mraspaud mraspaud self-assigned this Feb 15, 2021
@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels Feb 15, 2021
@djhoese
Copy link
Member

djhoese commented Feb 15, 2021

So what is the end result (ncdump -h?) of a typical netcdf written after these changes?

@mraspaud
Copy link
Member Author

Avhrr channel 1 for example:

	double \1(y, x) ;
		\1:_FillValue = NaN ;
		\1:calibration = "reflectance" ;
		\1:end_time = "2020-01-08 08:34:49.556000" ;
		\1:modifiers = "" ;
		\1:platform_name = "Metop-C" ;
		\1:resolution = 1050LL ;
		\1:sensor = "avhrr-3" ;
		\1:standard_name = "toa_bidirectional_reflectance" ;
		\1:start_time = "2020-01-08 08:19:49.890000" ;
		\1:units = "%" ;
		string \1:wavelength = "0.63 µm (0.58-0.68 µm)" ;
		\1:coordinates = "latitude longitude" ;

@djhoese
Copy link
Member

djhoese commented Feb 15, 2021

Why is wavelength the only string? Are the others getting encoded as bytes but wavelength is utf8 because of the micro?

@mraspaud
Copy link
Member Author

mraspaud commented Feb 15, 2021

yes, all the strings are passed as is to xarray, so it's happening there. See this example:

In [7]: ds = xr.Dataset(
   ...:     {"foo": (("x", "y"), np.random.rand(4, 5))},
   ...:     coords={
   ...:         "x": [10, 20, 30, 40],
   ...:         "y": pd.date_range("2000-01-01", periods=5),
   ...:         "z": ("x", list("abcd")),
   ...:     },
   ...: )
   ...: 
   ...: ds['foo'].attrs['noutf'] = 'hej'
   ...: ds['foo'].attrs['utf'] = 'hej på dig'
   ...: 
   ...: ds.to_netcdf("saved_on_disk.nc")
❯ ncdump -h saved_on_disk.nc
netcdf saved_on_disk {
dimensions:
	x = 4 ;
	y = 5 ;
variables:
	double foo(x, y) ;
		foo:_FillValue = NaN ;
		foo:noutf = "hej" ;
		string foo:utf = "hej på dig" ;
		foo:coordinates = "z" ;
	int64 x(x) ;
	int64 y(y) ;
		y:units = "days since 2000-01-01 00:00:00" ;
		y:calendar = "proleptic_gregorian" ;
	string z(x) ;
}

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.

Looks better than a block of JSON so there's that. 👍

Copy link
Collaborator

@TAlonglong TAlonglong left a comment

Choose a reason for hiding this comment

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

LGMT

@mraspaud mraspaud merged commit b792f29 into pytroll:master Feb 15, 2021
@mraspaud mraspaud deleted the feature-satpy-cf-id branch February 15, 2021 14:29
@gerritholl
Copy link
Collaborator

I get a unit test failure running latest satpy master on my system:

$ pytest -k test_wavelength_range_cf_roundtrip test_dataset.py
========================================= test session starts ==========================================
platform linux -- Python 3.8.6, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /home/gholl/checkouts/satpy
plugins: subprocess-1.0.0, socket-0.3.5, cov-2.11.1
collected 27 items / 26 deselected / 1 selectedtest_dataset.py F                                                                                [100%]=============================================== FAILURES ===============================================
__________________________________ test_wavelength_range_cf_roundtrip __________________________________    def test_wavelength_range_cf_roundtrip():
        """Test the wavelength range object roundtrip to cf."""
        from satpy.dataset.dataid import WavelengthRange        wr = WavelengthRange(1, 2, 3)>       assert WavelengthRange.from_cf(wr.to_cf()) == wr
E       AssertionError: assert WavelengthRan...3', unit='µm') == WavelengthRan...=3, unit='µm')
E
E         Omitting 1 identical items, use -vv to show
E         Differing attributes:
E         ['min', 'central', 'max']
E
E         Drill down into differing attribute min:
E           min: '1' != 1...
E
E         ...Full output truncated (8 lines hidden), use '-vv' to showtest_dataset.py:549: AssertionError
======================================= short test summary info ========================================
FAILED test_dataset.py::test_wavelength_range_cf_roundtrip - AssertionError: assert WavelengthRan...3...
=================================== 1 failed, 26 deselected in 0.90s ===================================

According to git bisect, this PR is to blame:

b792f293ff6d3d8db91245c60e60883c84e5f979 is the first bad commit
commit b792f293ff6d3d8db91245c60e60883c84e5f979
Author: Martin Raspaud <martin.raspaud@smhi.se>
Date:   Mon Feb 15 15:29:30 2021 +0100    Read wavelength ranges from netcdf (#1544):040000 040000 5f4cc6bfe1a9d6d2aecaee283e4400c08cf2126f 9f947c3e7b342d739687c7baf8374c3f3ed21c62 M     satpy

Why do I see this locally when tests pass in Github CI? I've updated to latest pyresample and pyspectral.

@gerritholl
Copy link
Collaborator

Updating trollsift resolved the failing test.

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.

None yet

4 participants