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 HydroPowerSimulations Docs #51

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Add HydroPowerSimulations Docs #51

wants to merge 27 commits into from

Conversation

rodrigomha
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.12%. Comparing base (8dc1711) to head (1a08f6a).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/feedforwards.jl 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
- Coverage   81.98%   79.12%   -2.86%     
==========================================
  Files           9        9              
  Lines         877      915      +38     
==========================================
+ Hits          719      724       +5     
- Misses        158      191      +33     
Flag Coverage Δ
unittests 79.12% <50.00%> (-2.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/core/constraints.jl 100.00% <ø> (ø)
src/core/expressions.jl 50.00% <ø> (+50.00%) ⬆️
src/core/formulations.jl 100.00% <ø> (ø)
src/core/initial_conditions.jl 100.00% <ø> (ø)
src/core/parameters.jl 46.15% <ø> (ø)
src/core/variables.jl 100.00% <ø> (ø)
src/hydro_generation.jl 74.83% <100.00%> (+1.22%) ⬆️
src/hydrogeneration_constructor.jl 91.38% <ø> (+0.61%) ⬆️
src/feedforwards.jl 60.95% <0.00%> (-29.53%) ⬇️

Copy link

@kdayday kdayday left a comment

Choose a reason for hiding this comment

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

Nice! I've added some comments based to improve formatting -- see best practices on general formatting. I think the extension packages would benefit from using DocumenterInterlinks.jl to other packages. Let me know if you need help. Thanks!

@@ -6,8 +6,8 @@ CurrentModule = HydroPowerSimulations

## Overview

`HydroPowerSimulations.jl` is a [`Julia`](http://www.julialang.org) package that provides blah blah
`HydroPowerSimulations.jl` is a [`Julia`](http://www.julialang.org) package that extends [`PowerSimulations.jl`](https://github.com/NREL-Sienna/PowerSimulations.jl) for modeling of hydro generation technology in production cost modeling simulations.
Copy link

Choose a reason for hiding this comment

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

Suggested change
`HydroPowerSimulations.jl` is a [`Julia`](http://www.julialang.org) package that extends [`PowerSimulations.jl`](https://github.com/NREL-Sienna/PowerSimulations.jl) for modeling of hydro generation technology in production cost modeling simulations.
`HydroPowerSimulations.jl` is a [`Julia`](http://www.julialang.org) package that extends [`PowerSimulations.jl`](https://nrel-sienna.github.io/PowerSimulations.jl/stable/) for modeling of hydro generation technology in production cost modeling simulations.

Copy link
Member

Choose a reason for hiding this comment

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

production cost modeling simulations -> operational simulations

Copy link

Choose a reason for hiding this comment

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

@rodrigomha, can you update the hyperlink above, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Miss that one

Copy link

Choose a reason for hiding this comment

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

Review the compiled html to remove all errors -- looks like PSCB isn't in the environment, and all code examples are failing

@@ -0,0 +1,62 @@
# [Operation Problem with `HydroPowerSimulations.jl`](@id op_problem)

**Originally Contributed by:** Rodrigo Henriquez-Auba
Copy link

Choose a reason for hiding this comment

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

I would remove

!!! note

`PowerSystemCaseBuilder.jl` is a helper library that makes it easier to reproduce examples in the documentation and tutorials. Normally you would pass your local files to create the system data instead of calling the function `build_system`.
For more details visit [PowerSystemCaseBuilder Documentation](https://nrel-sienna.github.io/PowerSystems.jl/stable/tutorials/powersystembuilder/)
Copy link

Choose a reason for hiding this comment

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

Link is failing due to PSY re-org. Either update, or preferably, set up DocumenterInterlinks.jl to PSY and use: [PowerSystemCaseBuilder Documentation](@extref powersystembuilder)

sys = build_system(PSITestSystems, "c_sys5_hy")
```

With a single `HydroDispatch`:
Copy link

Choose a reason for hiding this comment

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

Nice job breaking the code up into chunks. Please preface each call to a new function with a hyperlink to its docstring, e.g.,

With a single [`PowerSystems.HydroDispatch`](@extref)


## Decision Model

Setting up the formulations, including hydro using `HydroDispatchRunOfRiver`
Copy link

Choose a reason for hiding this comment

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

Maybe break this up to clarify what is coming standard from PSI, and reiterating the hydro part is from this extension package?

Copy link
Member

Choose a reason for hiding this comment

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

This one is defined here in HSS. The only formulation from PSI that's not defined here is FixedOutput

set_device_model!(template, HydroDispatch, HydroDispatchRunOfRiver)
```

```@repl op_problem
Copy link

Choose a reason for hiding this comment

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

Needs a text intro with hyperlinks

@@ -1,4 +1,12 @@
"""
Expression for HydroPumpedStorage that keep track
Copy link

Choose a reason for hiding this comment

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

Please turn this and all following references to HydroPumpedStorage into hyperlinks to the docstring using @extref: http://juliadocs.org/DocumenterInterLinks.jl/stable/

source - From where the data comes
affected_values -
- `component_type::Type{<:PSY.Component}` : Specify the type of component on which the Feedforward will be applied
- `source::Type{T}` : Specify the VariableType, ParameterType or AuxVariableType as the source of values for the Feedforward
Copy link

Choose a reason for hiding this comment

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

Add hyperlinks for all Types named in args lists

@@ -1419,6 +1419,9 @@ function PSI.construct_device!(
return
end

"""
Construct model for HydroGen with HydroCommitmentRunOfRiver Formulation
Copy link

Choose a reason for hiding this comment

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

Hyperlinks

Copy link
Member

@jd-lara jd-lara 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 good to me. The only needed thing is to address @kdayday's comments

@rodrigomha
Copy link
Contributor Author

Thanks for the comments: Here is the preview with the latest changes: https://nrel-sienna.github.io/HydroPowerSimulations.jl/previews/PR51/

@kdayday I did my best to add as much as possible for refs end extrefs. Hopefully I did not miss anything. I also realized that we do not have docstrings for ActivePowerBalance in PSI, so we will need to add those when we do the PSI docs update.

Copy link

@kdayday kdayday left a comment

Choose a reason for hiding this comment

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

@rodrigomha Looks good, -- I left 2 more comments/suggestions.

This closes #49, #50, correct? And any more updates to #38?

I also just added a new issue #52 to follow up on migrating the formulation library into the api -- please add any feedback there, thanks!

res = OptimizationProblemResults(model)
```

with dispatch variable results for the hydro:
Copy link

Choose a reason for hiding this comment

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

Suggested change
with dispatch variable results for the hydro:
Use [`read_variable`](@ref InfrastructureSystems.Optimization.read_variable) to read in the dispatch variable results for the hydro:

@rodrigomha
Copy link
Contributor Author

rodrigomha commented Dec 30, 2024

@rodrigomha Looks good, -- I left 2 more comments/suggestions.

This closes #49, #50, correct? And any more updates to #38?

I also just added a new issue #52 to follow up on migrating the formulation library into the api -- please add any feedback there, thanks!

I think this should also close #38, and yeah #50. Regarding #49, I'm not sure if this new HydroReservoir, is Pedro's new Reservoir model that we have not implemented yet.

@kdayday
Copy link

kdayday commented Dec 31, 2024

@rodrigomha Looks good, -- I left 2 more comments/suggestions.
This closes #49, #50, correct? And any more updates to #38?
I also just added a new issue #52 to follow up on migrating the formulation library into the api -- please add any feedback there, thanks!

I think this should also close #38, and yeah #50. Regarding #49, I'm not sure if this new HydroReservoir, is Pedro's new Reservoir model that we have not implemented yet.

Sounds good. Can you also update the formatter to close #38? Number 4 here: https://nrel-sienna.github.io/InfrastructureSystems.jl/dev/docs_best_practices/how-to/requirements_checklist/#For-Existing-Packages Thanks!

@rodrigomha
Copy link
Contributor Author

@rodrigomha Looks good, -- I left 2 more comments/suggestions.
This closes #49, #50, correct? And any more updates to #38?
I also just added a new issue #52 to follow up on migrating the formulation library into the api -- please add any feedback there, thanks!

I think this should also close #38, and yeah #50. Regarding #49, I'm not sure if this new HydroReservoir, is Pedro's new Reservoir model that we have not implemented yet.

Sounds good. Can you also update the formatter to close #38? Number 4 here: https://nrel-sienna.github.io/InfrastructureSystems.jl/dev/docs_best_practices/how-to/requirements_checklist/#For-Existing-Packages Thanks!

Thanks Kate. I updated to be like what we have in IS.

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.

Initial clean-up and reorganization of HydroPowerSimulations documentation
3 participants