Skip to content

Fixes SAM issue 994#1041

Merged
sjanzou merged 19 commits into
patchfrom
SAM_994
May 24, 2022
Merged

Fixes SAM issue 994#1041
sjanzou merged 19 commits into
patchfrom
SAM_994

Conversation

@sjanzou

@sjanzou sjanzou commented May 16, 2022

Copy link
Copy Markdown
Collaborator

No description provided.

sjanzou added 8 commits May 12, 2022 04:53
Also, when analysis period changes, reset to single value for consistent behavior in DataLifetimeMatrix. Can remove SetAnalysisPeriod from all callbacks. Can update DataLifetimeArray similarly
Working through code generation values not updated until DataLifetimeMatrix values are checked by reopening widget when analysis period changes.
SetAnalysisPeriod no longer used but callbacks not updated to prevent merge conflicts.
@sjanzou

sjanzou commented May 16, 2022

Copy link
Copy Markdown
Collaborator Author

Fix #994

Test file...
SAM_994.zip

Note that changing analysis period resets DataLifetimeMatrix to single value - consistent behavior now.
Also, note that when analysis period changes, the widget resets to single value when reopened and then the variable is updated. So, exporting code and running can be inconsistent when analysis period is changed if the merchant plant values are not rechecked. Finally, the SetAnalysisPeriod and the "analysis_period" property of the DataLifetimeMatrix is no longer used but left in the callbacks to prevent merge conflicts. We can remove and merge in a separate pull request.

@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.

Great job finding and fixing the bug!

Would it make sense to add a pop-up when users change the analysis period? I worry that reverting things to single value is not intuitive, and users would benefit from a warning that's what's happening. Am I correct that trimming the values to a shorter analysis period (or adding zeroes) was significantly more difficult?

Also, I'm having some trouble with the Cambium downloads for analysis periods different than 25 years:

image

Is this related or should it be a separate issue?

@sjanzou

sjanzou commented May 17, 2022

Copy link
Copy Markdown
Collaborator Author

Great job finding and fixing the bug!

Would it make sense to add a pop-up when users change the analysis period? I worry that reverting things to single value is not intuitive, and users would benefit from a warning that's what's happening. Am I correct that trimming the values to a shorter analysis period (or adding zeroes) was significantly more difficult?

Also, I'm having some trouble with the Cambium downloads for analysis periods different than 25 years:

image

Is this related or should it be a separate issue?

Definite separate issue - created #1042

@cpaulgilman

cpaulgilman commented May 18, 2022

Copy link
Copy Markdown
Collaborator

Would it make sense to add a pop-up when users change the analysis period? I worry that reverting things to single value is not intuitive, and users would benefit from a warning that's what's happening. Am I correct that trimming the values to a shorter analysis period (or adding zeroes) was significantly more difficult?

I agree with Brian's suggestion:

  • A message box that appears when you change the analysis period with text like "Analysis period change!\nThe merchant plant price data does not match the analysis period. Go to the Revenue page to update the Energy Market or Ancillary Service price data as appropriate.".

  • Preserve the mode and truncate or extend the pricing table in necessary to match analysis period instead of changing the mode to Single Value.

  • Remove UI callback code that sets the value of the widget's AnalysisPeriod property now that this is handled in the widget's C++ code.

Also, note that #1040 has changes to merchant plant UI forms.

@cpaulgilman

Copy link
Copy Markdown
Collaborator

@sjanzou These latest changes look good. I used a message box for the analysis_period on_change function so the red debt-related UI text warnings still work. The message box gives a strong reminder to check the price data when the user changes the analysis period.

@cpaulgilman cpaulgilman added this to the 2021.12.02 Patch 2 milestone May 20, 2022
@cpaulgilman cpaulgilman added the requires help revision Requires a Help revision before releasing public version label May 20, 2022
@brtietz

brtietz commented May 20, 2022

Copy link
Copy Markdown
Collaborator

The message works well, but I'm having some issues with multiple changes to the analysis period. Example:

  • Start a merchant plant case
  • Shorten the analysis period to 10 years
  • Energy Market widget has 87600 values as expected
  • Restore the analysis period to 25 years
  • Now there is a 30 minute timestep, with values 87601 - 438000 as zeroes

In the above case, I would expect the widget to maintain hourly values, with zeroes from 87601 - 219000. I was also able to produce the issue by lengthening to 30, then 40 years (that one went to 1 minute timesteps).

@sjanzou

sjanzou commented May 21, 2022

Copy link
Copy Markdown
Collaborator Author

The message works well, but I'm having some issues with multiple changes to the analysis period. Example:

  • Start a merchant plant case
  • Shorten the analysis period to 10 years
  • Energy Market widget has 87600 values as expected
  • Restore the analysis period to 25 years
  • Now there is a 30 minute timestep, with values 87601 - 438000 as zeroes

In the above case, I would expect the widget to maintain hourly values, with zeroes from 87601 - 219000. I was also able to produce the issue by lengthening to 30, then 40 years (that one went to 1 minute timesteps).

Hmmm... I am unable to duplicate:

  1. PVWatts / MP default 25 years hourly - 219000 rows

image

  1. Change analysis period to 10 years - stays hourly with 87600 rows

image

  1. Change analysis period to 25 years - resets to 219000 rows with zeros from 87601 to 219000

image

How did you produce a 30 minute timestep with 438000 rows?

Same thing when extending to 30 years and 40 years - expected behavior as outlined in last meeting and this pull request - what am I missing?
image

image

However, there is a major bug if the analysis period is changed twice before rechecking the revenue per the message
image

The number of rows in the DataLifetimeMatrix goes to the last analysis period / number of rows mode... working through that now and will request re-review after fixing.

@sjanzou

sjanzou commented May 24, 2022

Copy link
Copy Markdown
Collaborator Author

Items to test -

  1. multiple revenue prices with different durations - weekly, annual, hourly, subhourly
  2. multiple analysis period changes
  3. json or other code generation
  4. running without rechecking revenue price data after changing analysis period

defaults and export config and windows results included

@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.

The latest fixes work great, thank you!

@cpaulgilman cpaulgilman 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.

These latest fixes look great. It's hard to test the JSON and code generator exports because of the large arrays, but it seems to be working well. I will work on the Cambium issues after this is merged into Patch.

@sjanzou sjanzou merged commit 643f1bb into patch May 24, 2022
@sjanzou sjanzou deleted the SAM_994 branch May 24, 2022 21:48
@cpaulgilman cpaulgilman added added to release notes PR and/or issue has been added to release notes for a public release and removed requires help revision Requires a Help revision before releasing public version labels May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

added to release notes PR and/or issue has been added to release notes for a public release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merchant plant does not save percentage revenue variables with shorter analysis period

3 participants