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

Remove Compositor extra dims using composite_dims #407

Merged
merged 10 commits into from Jun 22, 2020

Conversation

jmilloy
Copy link
Collaborator

@jmilloy jmilloy commented Jun 10, 2020

Adds composite_dims trait and removes extra dimensions from the evaluation coordinates.

  • autodetect, ignore, or fail if the composite_dims are not provided?
  • tests

Autodetection could look like this at its simplest:

@tl.default("composite_dims")
    def _default_composite_dims(self):
        return self.sources[0].coordinates.udims

@jmilloy jmilloy changed the base branch from master to develop June 10, 2020 16:38
@jmilloy jmilloy marked this pull request as draft June 10, 2020 16:38
@jmilloy
Copy link
Collaborator Author

jmilloy commented Jun 10, 2020

@mpu-creare Another option instead of autodetecting the composite dims would just be to only remove extra dimensions if the composite_dims was defined. Maybe this is what you implemented as well.

@coveralls
Copy link

coveralls commented Jun 10, 2020

Coverage Status

Coverage increased (+0.004%) to 91.025% when pulling 91d8039 on feature/composite_dims into 2773e8d on develop.

@mpu-creare
Copy link
Contributor

Yes, that was exactly my intent. YOu should be able to autodetect 90% of the time, but for cases like TerrainTiles you can just drop the extra dimenions... or perhaps TerrainTiles is a special case that should just be handled there?

@mlshapiro
Copy link
Contributor

I don't think TerrainTiles is a necessarily a special case. It is an example of a dataset that does not have a time dimension, of which there may be many.

@mlshapiro
Copy link
Contributor

But yes - agree that auto-detecting makes a lot of sense!

@jmilloy thanks for getting right on it - let me know if you want me to help test / writes tests if you don't have time to work on this

@mpu-creare mpu-creare added the bug Something isn't working label Jun 11, 2020
@mpu-creare mpu-creare added this to In progress in 2.2.0 via automation Jun 11, 2020
@mpu-creare
Copy link
Contributor

I'm a little hesitant about the autodetection because of all the circular reference we run into whenever we have a default set from compositor.sources. Tagging dims as an attribute makes this fixable in any pipeline, so I'm happy with this. Works for my current case in any case.

Just needs tests then I'm happy to merge.

@jmilloy
Copy link
Collaborator Author

jmilloy commented Jun 11, 2020

Note

The obvious way to remove extra dimensions would be something like this:

extra = [dim for dim in coordinates.udims if dim not in self.dims]
coordinates = coordinates.drop(extra)

But if the compositor dims are lat, lon and the requested dims are lat, lon, time_alt, you have to remove the time_alt dimension together. On the other hand, if the requested dims are lat_lon_alt, you don't need to remove alt at all. So the removal is implemented like this:

extra = [
    c.name
    for c in coordinates.values()
    if (isinstance(c, Coordinates1d) and c.name not in self.dims)
    or (isinstance(c, StackedCoordinates) and all(dim not in self.dims for dim in c.dims))
]
coordinates = coordinates.drop(extra)

It is also implemented this way in DataSource.eval. In both the Compositor and DataSource tests, the lat, lon, alt_time case is tested, but the lat_lon_time case is not tested (commented out) because the interpolator cannot handle this case yet.

@jmilloy jmilloy requested a review from mpu-creare June 11, 2020 16:30
@jmilloy jmilloy marked this pull request as ready for review June 11, 2020 16:31
Copy link
Contributor

@mpu-creare mpu-creare left a comment

Choose a reason for hiding this comment

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

I have a few comments suggestions, but happy to merge.

podpac/core/compositor/ordered_compositor.py Show resolved Hide resolved
jmilloy and others added 4 commits June 11, 2020 14:43
I just commented this out for testing.
…mbda functions work out-of-the-box with the dev version of PODPAC.
…ain tiles to enable interpolation.

NOTE: I had to lightly round the coordinates of the terrain tiles because coordinate values used to index values in TileCompositor.get_data() were having float problems. The Coordinates.intersect() method was return coordinate values that were *slightly* different than the original coordinates, so the then when we tried to assign values in `output.loc[index] = source_data.values` xarray would fail to find the index. There is no way to realign here because `output` is a superset of `source_data`.
@mlshapiro
Copy link
Contributor

FYI I merged in my changes to Terrain Tiles and the TileCompositor here since it would conflict as a separate branch. See bc2bb16 for more information. Long story short, Terrain Tiles should now composite correctly. Whew. Thanks @jmilloy

…rk with APIGateway Lambda functions out of the box.
@jmilloy jmilloy merged commit 118b3f7 into develop Jun 22, 2020
2.2.0 automation moved this from In progress to Done Jun 22, 2020
@mpu-creare mpu-creare deleted the feature/composite_dims branch July 3, 2020 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants