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

Added weekly profile #72

Closed
wants to merge 39 commits into from

Conversation

s-simoncelli
Copy link
Contributor

Hello,

I implemented the weekly profile to support 52 (like pywr 1) or 53 values. Things to address:

  • The profile is implemented as Vec and not array due to the size limit of arrays
  • Import from v1 not yet implemented

@jetuk
Copy link
Member

jetuk commented Nov 23, 2023

Thanks for this.

Do we think the weekly profile should support interpolation like the monthly profile? I'm not suggesting this is done here, but if it would be a useful feature we can track it as an issue.

  • The profile is implemented as Vec and not array due to the size limit of arrays

Is this true? The daily profile is implemented as a 366 valued array.

You could make the difference between the two profile lengths an enum. This would force you to match on the type.

enum WeeklyProfile {
    FiftyTwo([f64; 52]),
    FiftyThree([f64; 53]),
}

pub struct WeeklyProfileParameter {
    meta: ParameterMeta,
    values: WeeklyProfile,
}

If you do the above you could then implement the look-up function as a method on the WeeklyProfile struct, and subsequently write a unit test in the module quite easily. Something like:

impl WeeklyProfile {
    fn value(&self, date: &time::Date) -> f64 {
        let current_day = date.ordinal() as usize;
        let current_day_index = current_day - 1;

        match self {
            Self::FiftyTwo(values) => {
                if current_day >= 364 {
                    values[51]
                } else {
                    values[current_day_index / 7]
                }
            }
            Self::FiftyThree(values) => {
                if current_day >= 364 {
                    values[52]
                } else {
                    values[current_day_index / 7]
                }
            }
        }
    }
}

@jetuk
Copy link
Member

jetuk commented Nov 23, 2023

This has still got some errors on compilation. If you need help resolving them let me know.

@jetuk jetuk mentioned this pull request Nov 30, 2023
72 tasks
@s-simoncelli
Copy link
Contributor Author

I completed the parameter. Now it also supports interpolation similarly to the MontlyProfileParameter with 52 or 53 weeks.
I also included unit tests and the documentation for the schema.

I wasn't 100% sure how to handle the conversion from Vec and I implemented From<Vec<f64>> in the WeeklyProfileSize enum.

@jetuk jetuk self-requested a review December 15, 2023 21:42
@jetuk
Copy link
Member

jetuk commented Dec 15, 2023

Thanks. I'll get around to reviewing this again in the next few days.

Copy link
Member

@jetuk jetuk 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 good. Some comments and some changes requested. I suggest we merge this with current functionality. Then, if we want to add ISO week functionality, do that in a separate update.

Doc string is very helpful and should be a template for the others.

pywr-core/src/parameters/profiles/weekly.rs Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Outdated Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Outdated Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Outdated Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Outdated Show resolved Hide resolved
pywr-core/src/parameters/profiles/weekly.rs Outdated Show resolved Hide resolved
pywr-schema/src/parameters/profiles.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
dependabot bot and others added 14 commits January 10, 2024 10:19
Updates the requirements on [polars](https://github.com/pola-rs/polars) to permit the latest version.
- [Release notes](https://github.com/pola-rs/polars/releases)
- [Commits](https://github.com/pola-rs/polars/commits)

---
updated-dependencies:
- dependency-name: polars
  dependency-type: direct:production
...

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Tomlinson <[email protected]>
Add `PywrMultiModel` schema and corresponding `MultiNetworkModel` to pywr-core. This update refactors the schema of the regular model to use a new "network" key. The schema then provides a `PywrNetwork` containing only the nodes, edges, parameters, metric sets and outputs. This is then reused by the regular model and the multi-model.

A new metric is added that can receive a metric from another model. This is implemented via the transfers in the multi-model configuration. This is fairly generic because it uses the metric system to allowing passing of node or parameter data from one model to another.
This now includes nested aggregation, hooking up the metric
sets to the model and use by the CSV recorder.

There's still some WIP but a general review would be good
at this point.
This requires implementing Clone for parameters' internal state
objects. For this a new super trait, ParameterState, is added that
requires Clone to be implemented. This is achieved via the
dyn_clone crate.

The Wasmer Store is not Clone which would block this function. As
the WASM parameter was a proof-of-concept this has been removed
for the time being in order to not block this functionality.
This is a refactor of NodeReference to specify a schema facing
attribute (NodeAttribute). This idea is to remove the need for
the user of the schema to understand the internals of complex
nodes. Instead they will specify a node by its name in the schema
and an associated NodeAttribute.

NodeAttribute is intended as a global enum of all possible values.
However, each node may only support a subset of these attributes.
I.e. some of the attributes do not make sense for certain node
types.

This has largely worked. However, there is an ugly requirement
in the control curves to specify the NodeReference struct
where it was previously just a string.
---
updated-dependencies:
- dependency-name: strum_macros
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

Fixed deprecated macro name in strum v0.26

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Tomlinson <[email protected]>
* chore(deps): Update polars requirement from 0.36.2 to 0.37.0

Updates the requirements on [polars](https://github.com/pola-rs/polars) to permit the latest version.
- [Release notes](https://github.com/pola-rs/polars/releases)
- [Commits](pola-rs/polars@rs-0.36.2...rs-0.36.2)

---
updated-dependencies:
- dependency-name: polars
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

* chore: Update pyo3 and pyo3-polars

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Tomlinson <[email protected]>
Remove the functions and replace with two wrapper classes
for Schema and Model. These have basic functionality to
deserialise, load and run a model.

Update Python 'Getting started' instructions in README.

Use PyO3/maturin-action for Python workflow.
Previously the node had two attributes, initial_volume and initial_volume_pc.
These are now variants of the enum.
An initial implementation of RollingVirtualStorage node.

- Adds support for rolling history in core virtual storage.
- Adds schema for RollingVirtualStorage.
- Implements conversion from v1 format.
- Basic test model.
- Add VirtualStorageNodeIndexNotFound error variant.
@jetuk
Copy link
Member

jetuk commented Feb 12, 2024

I'll look through this again. However, I'm not sure what's happened to the commits / branch. It is now showing a lot of changes to main?

@s-simoncelli
Copy link
Contributor Author

Hello,

I think I rebased the branch instead of merging the changes from the main. I then made the requested changes. Because it's going to be messy restoring the history, I created a clean branch here.

@jetuk
Copy link
Member

jetuk commented Feb 24, 2024

Closing in favour of #121

@jetuk jetuk closed this Feb 24, 2024
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.

3 participants