-
Notifications
You must be signed in to change notification settings - Fork 195
Sam 2105 use detailed batt w pvwatts #2113
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
Conversation
… on lifetime mode defaults
… for lifetime mode and new battery configs
Confusing for developers for this to be hidden behind `batt_bank_duration`.
Use `oldvalue()` to load simple battery inputs because those inputs are not in new detailed config. Add message to variables that are copied from old simple battery inputs. Set battery size units to AC to be consistent with simple battery. Replace `batt_bank_duration` with `batt_bank_size_ui` (note confusing widget `batt_bank_duration` was hidden under `batt_bank_size_UI`in UI) Revise message text.
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.
Please change the navigation tree label from "PV" to "PVWatts" to distinguish between Detailed PV and PVWatts models and for consistency with Hybrid configurations.
@brtietz I made some changes to the Version Upgrade script to fix the problems you described and a couple of others. Please test and let me know if you notice other issues. One unresolved item is that opening an old PVWatts-Battery file with lead acid battery does not correctly set the battery lifetime options. I think we want to load battery defaults for the chemistry and then apply the input from the old file, but not sure of the best way to do that. |
…se_detailed_batt_w_pvwatts
…hat it sets up the chemistry properly based on the previous option choses in PVWatts-Battery
@cpaulgilman Thanks for those fixes and feedback! I pulled the appropriate code from the UI forms into the version upgrade script to fully handle the chemistry transition between lead acid flooded and Li-ion NMC. This seemed better than hardcoding the variables in cmod_battwatts, and trying to force the upgrade script and UI form to work together (which resulted in "on load" overwriting user values when I tried it). Does this seem reasonable to you? I also found and fixed an error with the exclusive forms in the dispatch upgrades, and made the nav tree change you requested. |
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.
Thanks for these changes. Navigation menu looks good, and battery defaults are consistent with battery type.
Pull Request Template
Description
Changes:
Two areas where I could use help:
Fixes #753 and #2105
Corresponding branches and PRs:
Just SAM, develop for other repos.
Note that #2109 will require some additional defaults changes in these new files, depending on merge order.
Unit Test Impact:
Adds new configurations. Changes to lifetime mode mean defaults in test_results_win64 are expected.
Checklist
Reminders- this section can be deleted
[Checking for PySAM Incompatible API Changes]
(https://github.com/NREL/SAM/wiki/PySAM-Incompatible-API-Changes-&-Regenerating-PySAM-Files).
[When do the PySAM files need to be regenerated?]
(https://github.com/NREL/SAM/wiki/PySAM-Incompatible-API-Changes-&-Regenerating-PySAM-Files#when-do-the-pysam-files-need-to-be-regenerated-via-export_config)