Make coregistration metadata consistent and public

GlacioHack / xdem

Analysis of digital elevation models (DEMs)

https://xdem.readthedocs.io/

MIT License

122 stars 37 forks source link

Make coregistration metadata consistent and public #526

Closed rhugonnet closed 1 month ago

rhugonnet commented 1 month ago

This PR makes the .meta attribute a public property of Coreg, and makes the attribute names consistent.

In particular:

"offset_east_px" is now "shift_x", and is in georeferenced units (instead of pixels) which allows to remove the "resolution" key in the metadata,
"offset_north_px" is now "shift_y",
"vshift" is now "shift_z", Others are renamed to be consistent with BiasCorr.

Will be documented in #502!

Resolves #512

rhugonnet commented 1 month ago

@adehecq @erikmannerfelt Just want to quickly check that you agree with the new names before merging. Also, are we sure about meta? Fine with me, but could use something else if you have ideas.

rhugonnet commented 1 month ago

Merging to document in #502, can still make changes on your comments there before releasing

adehecq commented 1 month ago

@adehecq @erikmannerfelt Just want to quickly check that you agree with the new names before merging. Also, are we sure about meta? Fine with me, but could use something else if you have ideas.

I'm fine with the names used. But before it was clear that the unit of offsets was pixels, because it was in the name, now it's not as clear. I'm wondering if we should have a description somewhere of the possible items in "meta". Would it make sense to have a panda's dataframe instead with a column containing the value and another containing a description? I don't know if "meta" is the clearest name... By itself it means "beyond, above, transcending". Here I assume it was meant as a short name for metadata. I can't find a good alternative. We could use "params" but it is ambiguous with the parameters of the function. Or "results", but could be ambiguous with the coregistered DEM? Maybe "transformation", to indicate this is the applied transformation? I don't have a definite answer here sorry...