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

Multi band for MODIS #239

Merged
merged 3 commits into from
May 25, 2017
Merged

Multi band for MODIS #239

merged 3 commits into from
May 25, 2017

Conversation

Kirill888
Copy link
Member

@Kirill888 Kirill888 commented May 23, 2017

Reason for this pull request

To support modis collection 6 (MCD43A1) data files, or more generically HDF files that have multi-band datasets in them.

Proposed changes

  • Add extra parameter band in image/bands/{band,path,layer}.
    • When band parameter is present and contains an integer, read only that band.
    • Original behaviour when layer is used as band indicator still present
  • Updated modisprepare script and modis product definition
    • BRDF_Albedo_Parameters_xxx is now 3 layers BRDF_Albedo_Parameters_xxx_{iso,vol,geo}

Not done items

  • Tests added / passed
  • Fully documented, including docs/about/whats_new.rst for all changes

Adding extra field `band` to dataset description document. When set read that
specific band only. Previously `layer` was used as band when it was set to an
integer, however with MODIS data we need both `layer` as a string pointing to
the sub-dataset within a hdf file and also a band within the sub-dataset.
MODIS data set contains 3d parameter arrays like `BRDF_Albedo_Parameters_Band1`,
there are the whole bunch `BRDF_Albedo_Parameters_*` each one contains three
bands for `vol`, `geo` and `iso` parameters. Exposing those as individual layers
in datacube.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 81.124% when pulling 7978295 on multi-band-for-modis into e9b65f1 on develop.

@mpaget
Copy link
Contributor

mpaget commented May 23, 2017

Could you hold off merging this for a couple of days if practical, please? I'm updating and working through the modis c6 collections and might propose a more consistent (between modis products) approach to naming and structuring. I suspect your changes are quite valid; I'd just appreciate a couple of days to absorb them in the context of general refinements to the way modis is described and handled.

@Kirill888
Copy link
Member Author

No worries Matt,

won't be merged just yet, but do have a look at what I'm proposing, both code-wise and at the product description level. It's not a huge change, and is backwards compatible as far as code goes. Product modis_mcd43a1_tile is however affected in a way that will require re-indexing.

Also somebody needs to verify that I used the right names/bands for iso,vol,geo parameters.

@andrewdhicks andrewdhicks merged commit 3617474 into develop May 25, 2017
@woodcockr
Copy link
Member

Making a note, as I assume this happened whilst Matt was at GA today, that Matt confirmed his review for this change was consistent per his comments 2 days ago.

Could those accepting pull requests please get into the habit of noting that outstanding issues were resolved so others in the community know. Thank you.

@mpaget
Copy link
Contributor

mpaget commented May 25, 2017

Matt didn't make it to GA today - hopefully tomorrow. In off-git emails we agreed it was fine to proceed with the change to support landsat nbar requirements prior to Simon going on leave. We can always re-jig modis fields/labels/descriptions in the near future. In a way I was more flagging that I was actively working on the modis indexing side of things.

omad added a commit that referenced this pull request May 25, 2017
omad added a commit that referenced this pull request May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants