Skip to content

Improve REopt calls#1714

Merged
cpaulgilman merged 8 commits into
patchfrom
update-reopt-api
Feb 29, 2024
Merged

Improve REopt calls#1714
cpaulgilman merged 8 commits into
patchfrom
update-reopt-api

Conversation

@cpaulgilman

@cpaulgilman cpaulgilman commented Feb 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Pull Request Template

Description

Fix #1671 by reporting error message from API instead of causing SAM to crash. Requires building SAM-private to test.

Note that DimensionMismatch error from these test files is due to a bug in REopt API that the REopt team is fixing.

Improve REopt API calls:

  • Report error messages returned by API
  • Do not make API call unless simulation time step is 15, 30, or 60 minutes based on REopt documentation. See description of time_steps_per_hour at https://developer.nrel.gov/api/reopt/stable/help?api_key=DEMO_KEY.
  • Use progress bars to provide feedback: Separate progress bar for SAM simulation and API call.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This change modifies variables in existing compute modules. Please see Checking for PySAM Incompatible API Changes.

Checklist:

If you have added a new compute module in a SSC pull request related to this one, be sure to check the Process Requirements.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@cpaulgilman cpaulgilman added bug UI User interface issue that applies across performance and financial models labels Feb 27, 2024
@cpaulgilman cpaulgilman added this to the 2023 Release Patch 1 milestone Feb 27, 2024
@cpaulgilman cpaulgilman requested a review from brtietz February 27, 2024 18:32
@cpaulgilman cpaulgilman self-assigned this Feb 27, 2024
brtietz
brtietz reviewed Feb 28, 2024
View reviewed changes

@brtietz brtietz left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The new UI loading boxes look great! I'm having an issue where the REopt call always returns a zero battery size from this branch, even in cases where the battery has obvious value (PVWatts-Battery commercial), and cases where it returns a battery size in the released version. Did you see this as you were developing the branch? Or is this more likely to be a bug on their end?

@cpaulgilman

cpaulgilman commented Feb 29, 2024

Copy link
Copy Markdown
Collaborator Author

... I'm having an issue where the REopt call always returns a zero battery size from this branch, even in cases where the battery has obvious value (PVWatts-Battery commercial), and cases where it returns a battery size in the released version. Did you see this as you were developing the branch? Or is this more likely to be a bug on their end?

I also saw this. Sometimes rerunning REopt a few times results in a non-zero battery, so I'm not sure if it's SAM sending different data to the API or something on the REopt side. Also, the default case in this branch/Patch has different battery parameters than the default:

image

If you agree, I'd like to close this so we can fix the issue of REopt API calls causing SAM to crash, and investigate this as a separate issue for Patch 2.

@brtietz brtietz self-requested a review February 29, 2024 15:22
brtietz
brtietz approved these changes Feb 29, 2024
View reviewed changes

@brtietz brtietz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, digging into the sizing issues for patch 2 and merging this for patch 1 makes sense to me.

@brtietz brtietz mentioned this pull request Feb 29, 2024
Closed
@cpaulgilman cpaulgilman merged commit c2a3cc2 into patch Feb 29, 2024
@cpaulgilman cpaulgilman deleted the update-reopt-api branch February 29, 2024 16:21
@cpaulgilman cpaulgilman added the added to release notes PR and/or issue has been added to release notes for a public release label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brtietz brtietz brtietz approved these changes

Assignees

@cpaulgilman cpaulgilman

Labels

added to release notes PR and/or issue has been added to release notes for a public release bug UI User interface issue that applies across performance and financial models

Projects

None yet

Milestone

2023 Release Patch 1

Development

Successfully merging this pull request may close these issues.

REopt API call crashes SAM for weather file with subhourly data

2 participants

@cpaulgilman @brtietz