Skip to content
This repository was archived by the owner on Sep 28, 2023. It is now read-only.

Conversation

@jacobbieker
Copy link
Collaborator

@jacobbieker jacobbieker commented Oct 12, 2021

Pull Request

Description

Moved from openclimatefix-archives/nowcasting_utils#33

Adds positional encoders for:

  1. Absolute Position Encodings

It is generated using Fourier Features for now, although other options could include coordinates, such as CoordConv or others.

Fixes #4

How Has This Been Tested?

Unit Tests, which pass for the encoding. Some fail for the dataloader/datamodule, but that's unrelated to this

  • No
  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@jacobbieker jacobbieker added the enhancement New feature or request label Oct 12, 2021
@jacobbieker jacobbieker self-assigned this Oct 12, 2021
@jacobbieker jacobbieker force-pushed the jacob/position-encoding branch from 50d3c36 to 7b677d5 Compare October 13, 2021 09:00
@jacobbieker jacobbieker changed the title Add Positional Encoders Add Absolute Position Encoder Oct 13, 2021
@jacobbieker jacobbieker marked this pull request as ready for review October 14, 2021 07:55
Copy link
Collaborator

@flowirtz flowirtz left a comment

Choose a reason for hiding this comment

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

Few smaller comments, but after fixing that LGTM! 👍


Some [tests] fail for the dataloader/datamodule

Are we tracking those anywhere already?


def normalize_geospatial_coordinates(
geospatial_coordinates, geospatial_bounds: Dict[str, float], **kwargs
geospatial_coordinates: List[np.ndarray], geospatial_bounds: Dict[str, float], **kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

yay, more types!

@jacobbieker
Copy link
Collaborator Author

Few smaller comments, but after fixing that LGTM! +1

Some [tests] fail for the dataloader/datamodule

Are we tracking those anywhere already?

No, not right now, it seems like some changes in nowcasting-dataset meant the dataloading code I copied from SatFlow fails. Since nowcasting-dataset is still changing quite a bit with openclimatefix-archives/nowcasting_dataset#213 I was going with sorta waiting till that's more finished to fix the tests. I'll make pytest skip them actually for now

@jacobbieker
Copy link
Collaborator Author

Few smaller comments, but after fixing that LGTM! +1

Some [tests] fail for the dataloader/datamodule

Are we tracking those anywhere already?

No, not right now, it seems like some changes in nowcasting-dataset meant the dataloading code I copied from SatFlow fails. Since nowcasting-dataset is still changing quite a bit with openclimatefix/nowcasting_dataset#213 I was going with sorta waiting till that's more finished to fix the tests. I'll make pytest skip them actually for now

I'll be making these tests pass as part of #7

Copy link
Collaborator

@JackKelly JackKelly left a comment

Choose a reason for hiding this comment

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

Looks really great to me! Very nicely structured code - easy to read!

A few comments, mostly pretty small!



def encode_modalities(
modalities_to_encode: Dict[str, torch.Tensor],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should use an Enum for modality names, instead of strings? I'll start a new issue: #15

@jacobbieker jacobbieker merged commit 2de25e2 into main Oct 20, 2021
@jacobbieker jacobbieker deleted the jacob/position-encoding branch October 20, 2021 13:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add PV/Unified Position encoding

4 participants