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

Feature/profast fin model #355

Merged
merged 62 commits into from
Nov 8, 2024

Conversation

jaredthomas68
Copy link
Collaborator

@jaredthomas68 jaredthomas68 commented Oct 9, 2024

Add a ProFAST financial model to HOPP custom financial

This PR adds a basic ProFAST model to the HOPP custom financial model so all technologies can be modeled with ProFAST inside HOPP instead of using the PySAM financial models. While an effort has been made to align parameter names with the names in PySAM, I was not able to find corresponding parameters for all the profast parameters in PySAM.Singleowner.

Related issue

Impacted areas of the software

.github/workflows/ci.yml
.readthedocs.yaml
README.md
docs/requirements.txt
hopp/simulation/hopp_interface.py
hopp/simulation/hybrid_simulation.py
hopp/simulation/technologies/battery/battery.py
hopp/simulation/technologies/battery/battery_stateless.py
hopp/simulation/technologies/csp/csp_plant.py
hopp/simulation/technologies/dispatch/hybrid_dispatch_builder_solver.py
hopp/simulation/technologies/financial/custom_financial_model.py
hopp/simulation/technologies/grid.py
hopp/simulation/technologies/power_source.py
hopp/simulation/technologies/pv/detailed_pv_plant.py
hopp/simulation/technologies/pv/pv_plant.py
hopp/simulation/technologies/sites/site_info.py
hopp/simulation/technologies/wave/mhk_wave_plant.py
hopp/simulation/technologies/wind/wind_plant.py
pyproject.toml
tests/hopp/test_battery_dispatch.py
tests/hopp/test_custom_financial.py
tests/hopp/test_dispatch.py
tests/hopp/test_hybrid.py
tests/hopp/test_wave.py
tests/hopp/utils.py

Additional supporting information

Test results, if applicable

I changed the test value of two tests. One of them was off by less than 0.1% and I could not figure out what changed. The other required a change due to an added technology to the test.

@bayc bayc added enhancement New feature or request high priority Need to tackle soon labels Oct 23, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/requirements.txt Outdated Show resolved Hide resolved
@@ -374,6 +374,7 @@ def set_om_costs(self, pv_om_per_kw=None, wind_om_per_kw=None,
if self.battery:
if battery_om_per_kw:
self.battery.om_capacity = battery_om_per_kw
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two lines feel redundant? Why are we assigning battery_om_per_kw to both om_capacity and om_batt_capacity_cost?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. It looked like in some places the om_capacity may be used even for battery_om_per_kw. These are redundant names from my perspective that already existed in the code. I'm open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like om_capacity mirrors other technologies in hybrid_simulation.py, my vote is for that to be the one that's used instead of two different ones; I don't feel strongly about my preference though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dguittet is there a particular reason on the PySAM side that the battery has a parameter om_batt_capacity_cost and that this should be used over om_capacity? And that is that important in the custom financial model or more of a legacy effect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have submitted an issue for this discussion to continue: #401

hopp/simulation/technologies/csp/csp_plant.py Show resolved Hide resolved
},
)
pf.set_params("demand rampup", 0)
pf.set_params("long term utilization", 1) # TODO should use utilization
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to change this to the capacity factor and use the rated capacity for the set_param("capacity")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm inclined to keep it as is. Using capacity factor and nameplate capacity is essentially having ProFAST make the same calculations we already did to get the actual capacity. I'm open to discussion here, but let's make it a separate issue/PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to just make an issue about it, if we include multi-year simulations we will probably want to be able to add the dictionaries that include performance over time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have created an issue to continue this discussion: #402

pyproject.toml Outdated Show resolved Hide resolved
hopp/simulation/technologies/battery/battery_stateless.py Outdated Show resolved Hide resolved
tests/hopp/test_hybrid.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@johnjasa johnjasa left a comment

Choose a reason for hiding this comment

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

Looking good on my end, just a quick weigh-in comment! I'll avoid approving now; instead will wait for @kbrunik or others who have done a more complete review.


pf.set_params("maintenance", {"value": self.o_and_m_cost(), "escalation": gen_inflation})

# pf.add_fixed_cost(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Briefly looking at ProFAST examples, I see they include both set_params and add_fixed_cost calls in the same example. I think keeping the set_params calls and removing the commented code here is a good path forward assuming it mirrors the expected PF usage.

Copy link
Collaborator

@kbrunik kbrunik left a comment

Choose a reason for hiding this comment

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

I'm happy with where this PR is at, still more work to do overall but I think this one is looking good to go!

@johnjasa
Copy link
Collaborator

johnjasa commented Nov 7, 2024

Thanks @kbrunik! Is this ready to be merged in @jaredthomas68 or are you planning any further modifications? Could you please make issues for any upcoming changes that need to happen that are not part of this PR?

@jaredthomas68
Copy link
Collaborator Author

Thanks @kbrunik! Is this ready to be merged in @jaredthomas68 or are you planning any further modifications? Could you please make issues for any upcoming changes that need to happen that are not part of this PR?

I have created or referenced existing issues for all outstanding conversations. I'm going ahead with merging this PR now.

@jaredthomas68 jaredthomas68 merged commit 9b856a8 into NREL:develop Nov 8, 2024
3 checks passed
@jaredthomas68 jaredthomas68 deleted the feature/profast-fin-model branch November 8, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high priority Need to tackle soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants