-
Notifications
You must be signed in to change notification settings - Fork 103
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
Update for pysam hybrids #1091
Update for pysam hybrids #1091
Conversation
A financial compute module is currently required to run a hybrid compute module. We can revisit in the future. Required for the o and m and installation costs for each hybrid component separately in a hybrid system. The technology compute modules do not require the costs to run. The battery and fuel cell compute modules are run in lifetime mode regardless of the generator compute modules lifetime mode settings. |
@dguittet, are you working on this branch or a different one for patch 1? Thanks! |
@sjanzou Yes, this is the one still. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One test failing on Windows - all others are passing!
[ FAILED ] CmodHybridTest.GenericPVWattsWindFuelCellBatteryHybrid_SingleOwner
With latest changes, all ssc tests passing but all Hybrid FuelCell configurations are still failing to run per NREL/SAM#1584 |
ssc/cmod_hybrid.cpp
Outdated
if (compute_module == "generic_system") | ||
degrad = input.as_array("generic_degradation", &count_degrad); | ||
else | ||
degrad = input.as_array("degradation", &count_degrad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjanzou The reason the SAM tests are failing to run the Generic PVWatts Wind FuelCell Battery Hybrid system is because of these lines. The SAM ui page for the Generic System's AC degradation uses "degradation" for both Hybrid and regular Generic System cases. However, the SSC input to generic system is "generic_degradation". It seems like the generic_degradation is used within cmod_generic_system whereas degradation is used in the hybrid and financial compute modules?
Plot showing no "generic_degradation" in use
Anyways, is 'generic_degradation' used at all? In the Hybrid modules, we can just use "degradation" for all technologies that aren't fuelcell since that once has it's own "fuelcell_degradation". But then what if a user has different values for "generic_degradation" and "degradation"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generic_degradation is input to both cmod_generic_system and cmod_mhk_wave and is in the UI in Degradation AC Lifetime form as follows:
So, is used throughout the generic system.
Also, "degradation" is only used for non-lifetime simulation like PVWatts, pvsamv1 applies degradation outside of cmod_hybrid and is not used in cmod_hybrid (true for all generators with system_use_lifetime_output non-zero):
The code you showed for cmod_hybrid appears to be out of date... generic_degradation is not used in cmod_hybrid - when did you last merge patch into pysam_hybrids? Did it merge the latest from patch?
The latest patch branch does not have the lines 217 through 220 that you showed
https://github.com/NREL/ssc/blob/patch/ssc/cmod_hybrid.cpp
compute_module* cmod = static_cast<compute_module*>(p_mod); | ||
if (!p_mod) | ||
return 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment for why 35
@@ -120,7 +120,7 @@ cm_fuelcell::cm_fuelcell() | |||
add_var_info(vtab_fuelcell_input); | |||
add_var_info(vtab_fuelcell_output); | |||
add_var_info(vtab_technology_outputs); | |||
add_var_info(vtab_hybrid_tech_om); | |||
add_var_info(vtab_hybrid_tech_om_outputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move into hybridize?
ssc/cmod_hybrid.cpp
Outdated
var_data* financial_compute_modules = input_table->table.lookup("hybrid"); | ||
int analysisPeriod = (int)financial_compute_modules->table.lookup("analysis_period")->num; | ||
ssc_number_t inflation_rate = financial_compute_modules->table.lookup("inflation_rate")->num * 0.01; | ||
ssc_number_t sales_tax_rate = financial_compute_modules->table.lookup("sales_tax_rate")->num * 0.01; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment out and check tests
degrad = input.as_array("degradation", &count_degrad); | ||
ssc_number_t* degrad = input.as_array("degradation", &count_degrad); | ||
if (compute_module == "generic_system") | ||
input.assign("generic_degradation", *input.lookup("degradation")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjanzou Can check this? I think generic_system doesn't run lifetime, so degradation is used in cmod_hybrid and financials to extend out to lifetime
ssc/cmod_hybrid.cpp
Outdated
@@ -221,7 +239,7 @@ class cm_hybrid : public compute_module | |||
escal_or_annual(input, pOMLandLease, analysisPeriod, "om_land_lease", inflation_rate, total_land_area, false, input.as_double("om_land_lease_escal") * 0.01); | |||
} | |||
// optional fossil fuel costs | |||
if (compute_module_inputs->table.lookup("om_fuel_cost")) { | |||
if (compute_module_inputs->table.lookup("system_heat_rate")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trying to prevent seg fault of accessing the following variables
if (financial_compute_modules->table.is_assigned("ppa_multiplier_model")) | ||
compute_module_inputs->table.assign("ppa_multiplier_model", *financial_compute_modules->table.lookup("ppa_multiplier_model")); | ||
if (financial_compute_modules->table.is_assigned("ppa_price_input")) | ||
compute_module_inputs->table.assign("ppa_price_input", *financial_compute_modules->table.lookup("ppa_price_input")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reformat, and hopefully this makes sense and aligns with UI logic
Code generation for hybrids limited to only compute module variables.
Run all ssc tests
@sjanzou Ready for review now that tests are updated from code generator and passing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on windows and results matching with patch branches ssc and SAM.
Great work!
The input technologies to
cmod_hybrid
require some financial variables that aren't listed as required, and aren't checked by the cmod. In PySAM, this means if the user doesn't have these extra financial variables, the cmod will just have a hard exit. Fix this by making avtab_hybrid_tech_om_inputs
that isn't added to any of the compute modules, but is added dynamically bycmod_hybrid
to check that all these variables are present.Other changes to
cmod_hybrid
:exec
inflation_rate
andanalysis_period
from the "Hybrid" input_table which includes the financial cmods. The code originally requires each cmod to have its owninflation_rate
andanalysis_period
which is problematic because not all technology cmods have these as inputs, and it seems to make it possible to have different values. However, this requires there to be a financial cmod to the hybrid cmod, and this is currently not enforced. Should a financial model be required for running a hybrid cmod?system_use_lifetime_output
, such aswindpower
, so instead of adding a new interface variable to these cmods, I made cmod_hybrid assume False unless the variable is available and True.Happy to discuss concerns about any of these proposed changes.