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

Add method to load data sources into the model. #532

Merged
merged 36 commits into from
Jan 22, 2024

Conversation

brynpickering
Copy link
Member

@brynpickering brynpickering commented Jan 6, 2024

Fixes #92

Summary of changes in this pull request:

  • data_sources top-level key to allow loading arbitrary data from file.
  • example of use with the national-scale model.
  • parent param -> base_tech.
  • Remove file=/df= functionality.
  • Tutorial notebook showing different use-cases on a simple model.

This implementation has required some code in calliope.preprocess.model_data to align data loaded from file and those from YAML. This would ideally be even cleaner than it is, but it works for now. The approach I'm taking is:

  1. Load all data from file into one dataset
  2. Create a dummy dict from this with empty tech definitions and empty node definitions with relevant techs attached to those nodes (based on what data is defined from file). Both of these are applied to the user-defined YAML to not mess up the YAML->dataset code.
  3. Create another dummy dict with base tech data (base_tech and carrier_in/out - if included in data from file) which goes at the bottom of the tech inheritance chain. I didn't put this in (2) as I don't want it to override some user YAML definition (e.g., carrier_in is changed by a YAML override compared to what is loaded from file).

## REMAINING ISSUES
- Currently, It is very difficult to ensure any amount of YAML definition can be handled. If there is minimal info provided in YAML (e.g., one specific parameter override for one tech) then you have no info available about which techs exist at which nodes except for what you have provided in data_sources. For the national scale example I've set up, inferring which techs are defined at which nodes gets messed up by array broadcasting of flow_cap_max, making the model think that all techs are defined at all nodes. EDIT: I think this is fixed.
~~- We probably don't want the national scale example data duplicated in CSV in the calliope module itself. Perhaps we move this to tests? ~~

TODO

- [ ] Order of overrides (YAML > data sources or data sources > YAML) and exception behaviour on clashes between data sources and between YAML and data sources (both currently set to silently override) should be configurable. EDIT: leaving "YAML > data sources" order as-is an non-configurable.

  • Would be easy enough to extend the loading from file to be direct from netcdf and / or from excel (just add a sheet_name param).
  • tests
  • docs

Reviewer checklist:

  • Test(s) added to cover contribution
  • Documentation updated
  • Changelog updated
  • Coverage maintained or improved

The method for loading them in isn't dirty,
but aligning YAML definitions with those from file is.
@brynpickering brynpickering marked this pull request as ready for review January 7, 2024 13:06
Dummy test model isn't well-formatted to pass the carrier in/out checks.
We _do_ check these in the YAML schema so that should be sufficient.
Copy link

codecov bot commented Jan 7, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (33b0672) 95.19% compared to head (e4a7137) 95.65%.

❗ Current head e4a7137 differs from pull request most recent head 15f102c. Consider uploading reports for the commit 15f102c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #532      +/-   ##
==========================================
+ Coverage   95.19%   95.65%   +0.46%     
==========================================
  Files          24       25       +1     
  Lines        3306     3450     +144     
  Branches      706      683      -23     
==========================================
+ Hits         3147     3300     +153     
+ Misses         92       85       -7     
+ Partials       67       65       -2     
Files Coverage Δ
src/calliope/attrdict.py 96.48% <100.00%> (ø)
src/calliope/backend/backend_model.py 97.67% <100.00%> (+0.01%) ⬆️
src/calliope/core/io.py 94.79% <100.00%> (+0.34%) ⬆️
src/calliope/core/model.py 94.76% <100.00%> (+0.05%) ⬆️
src/calliope/examples.py 100.00% <100.00%> (ø)
src/calliope/postprocess/postprocess.py 90.32% <100.00%> (ø)
src/calliope/preprocess/data_sources.py 100.00% <100.00%> (ø)
src/calliope/util/schema.py 90.32% <100.00%> (ø)
src/calliope/preprocess/model_data.py 99.34% <97.77%> (-0.66%) ⬇️
src/calliope/preprocess/time.py 95.78% <95.23%> (+7.21%) ⬆️

... and 1 file with indirect coverage changes

src/calliope/attrdict.py Show resolved Hide resolved
src/calliope/config/config_schema.yaml Outdated Show resolved Hide resolved
src/calliope/config/config_schema.yaml Outdated Show resolved Hide resolved
src/calliope/config/config_schema.yaml Outdated Show resolved Hide resolved
src/calliope/config/model_data_checks.yaml Outdated Show resolved Hide resolved
src/calliope/example_models/national_scale/scenarios.yaml Outdated Show resolved Hide resolved
src/calliope/core/model.py Outdated Show resolved Hide resolved
src/calliope/example_models/national_scale/scenarios.yaml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a pretty confusing approach that will rapidly become error-prone and difficult to manage with larger models. We should at least try to show an example where YAML and CSV are mixed and not highlight this as the "default" approach?

@brynpickering
Copy link
Member Author

brynpickering commented Jan 10, 2024

@sjpfenninger for carrier_in and carrier_out as well as to and from, perhaps we can let them work in a special way...

carrier_in/_out:

The user could provide it without the carriers dimension as they do in YAML. E.g.:

techs carrier_in carrier_out
supply_tech foo
supply_tech bar
demand_tech baz
conversion_tech [foo, bar] baz

We would then enforce that when loading the data and process it back into a dictionary to merge into the traditional model definition.

Multiple carriers in a list would need special parsing as they would likely be loaded in as strings ("[foo, bar]") and would need processing back to lists of strings.

to/from

As with carrier_in/_out, we let users define it as they would in YAML. However, there is the added step that we would need to identify transmission technologies once all data files are loaded and a dummy "traditional" model definition has been created. Then we would go back to the loaded data files and make a check that no parameters were defined over the nodes dimension for transmission technologies. This would then emulate the YAML loading checks, which do not allow a transmission technology to be defined at a node.

The loop back to do the nodes check could be a pain, but manageable I think.

pros/cons

pros

  • it maps to what the user does in YAML
  • it allows users to define link to/from nodes in a way that is clearer than defining them via carrier_in and carrier_out

cons

  • it's more work on our side and therefore a higher maintenance burden
  • might be confusing to the user that carrier_in/_out are pivoted in model.inputs to have the values as one of the dimensions and the values being binary.
  • It means that you can't use a pre-built calliope model as the data source to your model. Imagine a "vanilla" calliope model being loaded in as a data source and then a set of YAML-defined overrides to tweak that data. It wouldn't work because Calliope would see a malformed carrier_in/_out and would be missing to and from.

@brynpickering
Copy link
Member Author

@sjpfenninger the solution I opted for was to allow to/from to be defined in text format in file but to limit carrier_in/out to still be boolean and to raise an error if a transmission technology defines data at nodes in the loaded data from file. This seems to me like a reasonable compromise that stops us having a separate method to load data from file (as in, YAML-esque data that needs to be processed separately to a dictionary).

@brynpickering
Copy link
Member Author

Docs added in #538

Copy link
Member

@sjpfenninger sjpfenninger left a comment

Choose a reason for hiding this comment

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

A few minor comments/suggestions.

Besides the remaining issue of dealing with carrier_in and carrier_out, which probably takes a bit more thinking to resolve (if it doesn't just stay as-is) I think this looks good now and ready to merge in for the beta.

docs/examples/calliope_model_object.py Outdated Show resolved Hide resolved
src/calliope/config/model_data_checks.yaml Outdated Show resolved Hide resolved
src/calliope/config/protected_parameters.yaml Outdated Show resolved Hide resolved
src/calliope/config/protected_parameters.yaml Outdated Show resolved Hide resolved
src/calliope/config/protected_parameters.yaml Outdated Show resolved Hide resolved
tests/common/national_scale_from_data_sources/model.yaml Outdated Show resolved Hide resolved
tests/common/national_scale_from_data_sources/model.yaml Outdated Show resolved Hide resolved
docs/hooks/macros.py Show resolved Hide resolved
docs/examples/urban_scale/index.md Outdated Show resolved Hide resolved
@brynpickering brynpickering merged commit 60b0df8 into main Jan 22, 2024
7 of 8 checks passed
@brynpickering brynpickering deleted the feature-load-from-data-sources branch January 22, 2024 19:06
brynpickering added a commit that referenced this pull request Jan 22, 2024
* Add `data-sources` top-level key to load tabular data into the model from file or pandas.DataFrame.
* Remove `file/df=` functionality.
* Fix resampler; add parameter dtype casting.
* parent -> base_tech.
* Add data source schema validation.
* Add tutorial to docs.

---------

Co-authored-by: Stefan Pfenninger <stefan@pfenninger.org>
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.

Load non-timeseries data from file
2 participants