nasa / EMIT-Data-Resources

This repository provides guides, short how-tos, and tutorials to help users access and work with data from the Earth Surface Mineral Dust Source Investigation (EMIT) mission.
Apache License 2.0
138 stars 81 forks source link

Add wavelength lookup for bands #25

Closed alexgleith closed 1 year ago

alexgleith commented 1 year ago

This PR adds an index for the bands dimension, which enables selecting a band simpler than is happening in the examples now.

For example, rather than doing: np.nanargmin(abs(ds["wavelengths"].values - band))

You can now do: ds.sel(bands=450, method="nearest")

This simplifies a lot of things, including plotting with holoviews. Example captured below in an image.

image

alexgleith commented 1 year ago

Note that this is a breaking change, actually. It will require example notebooks to be updated, but I think that's worth doing.

ebolch commented 1 year ago

I've been looking into this and think we want to keep the 'bands' as a dimension. I'm trying to make sure the functions in the module work with all of the EMIT products, some of which don't have wavelengths as a coordinate.

There is a better way than the np.nanargmin(abs(ds["wavelengths"].values - band)) example that I put into the notebooks. For single dimension coordinates you can add an index that will work with the sel function. image This will also work with the mask dataset. (no clouds in this scene) image

I'm working on some updates to emit_tools.py and I'll update the notebooks with this example soon.

alexgleith commented 1 year ago

The change I made here does keep it as a dimension, just adds an index.

Is there a downside to having an index? I can only see positive impacts of adding the index!

ebolch commented 1 year ago

We had a discussion about this and are considering setting 'wavelengths' as the dimension in the xarray object instead of 'bands' for the radiance and reflectance products, since 'bands' is ambiguous. This would have the functionality you mentioned. Example: image image

alexgleith commented 1 year ago

This is a great idea, I approve!

Also consider making it "wavelength" since others are in the singular too, I.e., latitude, longitude, time...

ebolch commented 1 year ago

Will keep that in mind. Going to spend a good amount of time making some changes next week.

amfriesz commented 1 year ago

@alexgleith, @ebolch has implemented the updates he described on June 23rd. I've just merged them into the main branch. Is there still a need for your PR when these changes implemented?

alexgleith commented 1 year ago

Hey @amfriesz no worries, I'll close this.

Thanks for making the changes, I'm sorry but I don't have time to test them now but they look sensible and helpful.