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

feat: Upgrade to pywr-v1-schema v0.8.0. #66

Merged
merged 4 commits into from
Nov 25, 2023
Merged

feat: Upgrade to pywr-v1-schema v0.8.0. #66

merged 4 commits into from
Nov 25, 2023

Conversation

jetuk
Copy link
Member

@jetuk jetuk commented Oct 30, 2023

Required implementing several new parameters:

  • InterpolatedParameter
  • DiscountFactorParameter
  • DataFrameParameter

Conversion from Storage, Deficit and Flow parameters is now returns an error as these should become metrics in pywr-next.

Todos are left for HydropowerTarget, RollingMeanFlowNode and ScenarioWrapper parameter.

Also the control curve interpolated parameter was renamed due to a naming conflict with InterpolatedParameter.

Required implementing several new parameters:

- InterpolatedParameter
- DiscountFactorParameter
- DataFrameParameter

Conversion from Storage, Deficit and Flow parameters is now
returns an error as these should become metrics in pywr-next.

Todos are left for HydropowerTarget, RollingMeanFlowNode and
ScenarioWrapper parameter.

Also the control curve interpolated parameter was renamed
due to a naming conflict with InterpolatedParameter.
@jetuk jetuk requested a review from Batch21 October 30, 2023 19:11
@jetuk jetuk mentioned this pull request Oct 30, 2023
2 tasks
Copy link
Contributor

@Batch21 Batch21 left a comment

Choose a reason for hiding this comment

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

Couple of queries but otherwise this looks good

})
.collect::<Result<_, TableError>>()?;

Ok(LoadedScalarTable::Three(ScalarTableThree { values }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this return a LoadedScalarTable::Two and another function added to create a LoadedScalarTable::Three?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yes. Not sure how this is compiling!

.collect::<Result<_, TableError>>()?;

Ok(LoadedVecTable::Two(tbl))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this missing a load_csv_row3_vec_table_one function?

@jetuk
Copy link
Member Author

jetuk commented Nov 9, 2023

This was a bit broken. I've fixed the data_tables module. At least to the extent it has the same functionality. It could do with completing other cases and having some extensive tests & documentation.

@jetuk jetuk force-pushed the pywr-schema-0_8_0 branch from 77639b8 to f73def3 Compare November 9, 2023 21:55
@jetuk jetuk requested a review from Batch21 November 13, 2023 14:59
Copy link
Contributor

@Batch21 Batch21 left a comment

Choose a reason for hiding this comment

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

fixes look good

@jetuk jetuk merged commit d3dca15 into main Nov 25, 2023
2 checks passed
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.

2 participants