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

[POC] add metrics from demcompare #602

Open
4 tasks
adebardo opened this issue Aug 23, 2024 · 2 comments · May be fixed by #638
Open
4 tasks

[POC] add metrics from demcompare #602

adebardo opened this issue Aug 23, 2024 · 2 comments · May be fixed by #638

Comments

@adebardo
Copy link

adebardo commented Aug 23, 2024

Context

The purpose of this ticket is to implement the demcompare metrics into geoutils. We have decided not to include them in xdem because these measurements apply specifically to rasters.

Code

We propose first to copy the metric directory from demcompare into geoutils.

Example script:

import geoutils as gu  
from geoutils.metric import Metric  
  
filename_rast = gu.examples.get_path("exploradores_aster_dem")  
rast = gu.Raster(filename_rast)  
  
metric_obj = Metric("pdf")  
output = metric_obj.compute_metric(rast.data)  
  
print(output)

To do:

  • Remove the matrix_2d_metrics file and open a ticket to move the SVF to the terrain attributes in xdem (also check the init file).
  • Recode the compute_surface_normal and remove_nan_and_flatten functions in geoutils.

Tests

For testing, you can retrieve and adapt the following files:

Documentation

A section should be added inspired by the demcompare documentation.

@duboise-cnes
Copy link
Member

I understand the previous discussion to go to geoutils for the simple metrics and to have metrics on geoutils, but i see now a dichotomy with matrix_2D metrics (svf, hillshade) that are kept in xdem.

I feel here we split the metrics in geoutils and xdem, I feel that we will have difficulty to have a common unique way of automating metric computation through a CLI pipeline (such as demcompare). It was my question in : GlacioHack/xdem#551
and I don't know if you have discussed this matter.
I would have thought that the overall Metric structure would be in xdem and this structure could call geoutils sub functions for "simple" raster if interesting to have them in
Not sure with these splits.

In demcompare pipeline, we can choose also svf, hillshade metric to generate them based on pipeline configuration file. I don't know if I am clear, please tell me if not.

So before finalizing this function, I think it could be important to be sure how we plan to use this structure from user and pipeline point of view and see if this design match. To be discussed...

for compute_surface_normal and remove_nan_and_flatten in demcompare, it was not well architectured, so take the easier approach with xdem/geoutils philosophy in mind. Geoutils seem the best way, i agree.

@rhugonnet
Copy link
Member

I did not fully realize that under "metrics" were many different functions: from a single value metric (mean, STD), to a distribution (PDF) and visualization (hillshade). I think we might have only shortly mentioned this during our discussion in August, and I agree with @duboise-cnes that in that case, wrapping all of this directly in xDEM would make the most sense.

Additionally, it seems the primary use of the Metric class would be to link the CLI with a functionality that already exists separately in the Python API (whether it's a NumPy reduction function like np.mean(), or terrain attribute like terrain.hillshade()). In that case, it seems the Metric class would not be useful in the Python API, but only as a CLI link, and thus we could make it non-public. That would be further justification to have everything implemented in xDEM (as it would bring little value to GeoUtils if it is non-public; at least as long as we don't require a CLI in GeoUtils... but currently we don't have any pipeline there, like we have coregistration for DEMs...).

@vschaffn vschaffn linked a pull request Dec 4, 2024 that will close this issue
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 a pull request may close this issue.

3 participants