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

Align DataSource constructor args with config YAML field names #285

Merged

Conversation

JackKelly
Copy link
Member

@JackKelly JackKelly commented Oct 26, 2021

Pull Request

Description

Modify the DataSource constructor arguments and YAML config field names so they align precisely.

This PR touches a lot of files because it renames a few argument names. But the actual underlying conceptual change is (hopefully!) quite small!

I haven't renamed all the variables in datamodule.py and prepare_ml_data.py because these files will be removed soon!

This is one small step towards the "big new design" described in issue #213.

  • Implement DataSourceList.from_config(). The main reason for aligning the DataSource constructor args with the YAML config field names is so we can create this simple little function to create every DataSource from the YAML config 🙂
  • Instead of using the first entry in the DataSourceList to define the geospatial location of each example, instead use a new DataSourceList.data_source_which_defines_geospatial_locations attribute. This is more explicit and makes the code a little easier. But I've also made the code backwards-compatible for now.
  • Align YAML config field names with DataSource constructor arguments:
    • Rename these fields in the config YAML:
      • solar_pv_data_filename -> pv_filename
      • solar_pv_metadata_filename -> pv_metadata_filename
      • sat_channels -> satellite_channels
    • Rename these DataSource constructor args:
      • GSPDataSource.filename -> GSPDataSource.zarr_path
      • SatelliteDataSource.filename -> SatelliteDataSource.zarr_path
      • SunDataSource.filename -> SunDataSource.zarr_path
  • Test DataSourceList.from_config().

Related issues:

How Has This Been Tested?

The tests pass

  • 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

@JackKelly JackKelly self-assigned this Oct 26, 2021
@JackKelly
Copy link
Member Author

JackKelly commented Oct 26, 2021

@peterdudfield I remember we had previously discussed keeping the verbose, DataSource-specific names for each configuration parameter (like nwp_channels and sat_channels) but, after making a start on modifying the DataSources to use these constructor arguments, it feels like it does add quite a lot of complexity to the code. So, for now, I'm going to go the other way, and use standard field names in the config file, so the downstread code can be as simple as possible. We can always revert back if you want!

@JackKelly
Copy link
Member Author

Actually, ignore my comment above, I've just thought of a simple way to use verbose config names!

@JackKelly JackKelly requested a review from jacobbieker October 26, 2021 15:13
@JackKelly JackKelly marked this pull request as ready for review October 26, 2021 15:13
Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

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

LGTM!

@JackKelly
Copy link
Member Author

Thanks for the quick review, @jacobbieker!

@JackKelly JackKelly merged commit 550576d into main Oct 26, 2021
@JackKelly JackKelly deleted the jack/align-DataSource-constructor-args-with-YAML-config branch October 26, 2021 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Change each DataSource subclass' constructor arguments to align with the new YAML config field names
2 participants