Skip to content

Refactoring Fourier metrics#30

Merged
martinjanssens merged 7 commits intocloudsci:masterfrom
martinjanssens:refactor-fourier
Jul 29, 2021
Merged

Refactoring Fourier metrics#30
martinjanssens merged 7 commits intocloudsci:masterfrom
martinjanssens:refactor-fourier

Conversation

@martinjanssens
Copy link
Collaborator

@leifdenby this one might require us to agree on format as well. Currently, I'm writing the Fourier metrics as operating on a power spectral density, that has previously been computed, following the terminology:

spectra = cloudmetrics.compute_spectra(cloud_scalar,...)
metric_x = cloudmetrics.spectral_metric(spectra)

This allows us to only compute the spectra once before computing the metrics that derive from those spectra. It's a bit explicit and also deviates from the metric(cloud_scalar) terminology, but I think this might be clearer. I'm also adding a compute_all(cloud_scalar) function that wraps spectrum calculation and all the individual metric calculations.

@martinjanssens martinjanssens requested a review from leifdenby July 27, 2021 09:06
Copy link
Collaborator

@leifdenby leifdenby left a comment

Choose a reason for hiding this comment

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

This looks really great @martinjanssens, really great.

Regarding avoiding doing the computations more than once we could do some caching (like I do with the object scales. This isn't so pretty, but it works.

I'm a bit unsure about the structure of package, but as we're working through things the picture becomes clearer :) It seems we have quiet a few different levels where a user might call cloudmetrics... since the most general process at the moment seems to be:

  1. Take a scalar field
    • compute metric on scalar field, or
  2. calculate mask from scalar field
    • compute metric on mask, or
  3. produce object labels from mask
    • compute metric on object

It could be nice to allow the user to start from any of these three points, but I'm not sure what the best layout would be. Maybe cloudmetrics.from_mask.{metric_name}, cloudmetrics.from_scalar.{metric_name} or cloudmetrics.from_labels.{metric_name}?

We can continue this discussion later though. Maybe we should merge in this pull request and the work out how to restructure the package once we've got them all in?

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.

2 participants