Fixes SAM #496

[sjanzou]

No description provided.

[brtietz]

Changes look good overall, though it looks like the test is failing (good to have a test for this!). Generally, I'd prefer to have the commented out lines removed to avoid future confusion.

[brtietz]

Please remove commented out code (leaving the bit about the int causing an overflow is fine)

[dguittet]

I hate to be the annoying one here but I can't help but think that this fix just pushes the overflow issue onto doubles, rather than determining how SAM should handle overflows, which are by language, undefined.

One can still have negative numbers, or Inf values given a large enough input:

This might be a bit simple-minded, but could the overflow in the battery and PV have been dealt with by making an error check in the callback? Like if any of the values are < 0, or Inf, please adjust the desired sizes? The GUI objects should have Int formatting when the SSC value is an int, and then the overflow can be detected in the GUI, leaving the SSC code unchanged. Additionally, if we wanted to increase the range, we can switch ints to long long.

[sjanzou]

@dguittet, yes, this "fix" was to convert 19 key variables from int to doubles throughout the UI and in all calculations in the PV and Battery libraries and compute modules as detailed in the above Word document.
The callback check would do nothing for SDK users which would get unpredictable results... Several of the integer UI variables were typecast into size_t or doubles throughout the battery model and then again implicitly typecast into function arguments.
I agree that a systematic approach to perform all compute module calculations using a single type, ssc_number_t, would be best.
Also, the UI display when using wxUINumeric controls and showing as integer, further exacerbates the issue by using wxAtoi which does an integer type cast limiting the values more.

[sjanzou]

The data type ranges can be viewed at https://docs.microsoft.com/en-us/cpp/cpp/data-type-ranges?view=msvc-160 (for our predominant platform). The int to long long is okay but does not address the mixture of previous typecasting in ssc that is in patch and develop. Also, the wex library would have to be updated to display properly to whatever we chose as our standard going forward.

[sjanzou]

The failing test will be addressed after we decide on a path forward...

[dguittet]

@dguittet, yes, this "fix" was to convert 19 key variables from int to doubles throughout the UI and in all calculations in the PV and Battery libraries and compute modules as detailed in the above Word document.
...
I agree that a systematic approach to perform all compute module calculations using a single type, ssc_number_t, would be best.

While I see your point that we should stay with consistent types, I don't think making everything a ssc_number_t makes sense because there are discrete variables and should represented as such, not as doubles. I'm worried this will lead to unintended problems. And you're absolutely right that my suggestion of using long long in some places and int in others will create a big problem.

The callback check would do nothing for SDK users which would get unpredictable results... Several of the integer UI variables were typecast into size_t or doubles throughout the battery model and then again implicitly typecast into function arguments.

That's true on the battery model side, and suggests we add error checks around those implicit typecasting lines and function and throw reasonable errors back to both the SDK and SAM users.

Also, the UI display when using wxUINumeric controls and showing as integer, further exacerbates the issue by using wxAtoi which does an integer type cast limiting the values more.

Perhaps this reinforces that long long is not an option, which is ok with me.

[dguittet]

Re-starting this convo for patch 2, I still don't think we should be using doubles to represent values that are inherently integers.

In SSC, we can add a check that these integer inputs are > 0 and then throw an exception so that SAM can properly notify the user.

In SAM, we'll need to add a callback function to protect against negative overflow if we don't want to even display these negative values to the user.

[sjanzou]

Re-starting this convo for patch 2, I still don't think we should be using doubles to represent values that are inherently integers.

In SSC, we can add a check that these integer inputs are > 0 and then throw an exception so that SAM can properly notify the user.

In SAM, we'll need to add a callback function to protect against negative overflow if we don't want to even display these negative values to the user.

@brtietz , @dguittet, @cpaulgilman
While the quoted solution does address the original issue reported in #496, the solution limits the number of cells in the battery model and; hence, the desired battery bank capacity more than at least one user requested.
We can report the issue back to the users and stay with int values and assume 64-bit ints across platforms or we can discuss further.

[dguittet]

@sjanzou Good point about the size limitations. This can be resolved by using larger cells. The default cell sizes are rather small and probably unrealistic building blocks for MWh+ battery systems...

[brtietz]

I don't think it's necessary to support sizes above max int. 2.1TWh is ~500x the annual installations for the entire US. If they want to model systems that large they should be using ReEDS, not SAM.

[brtietz]

Closed as per discussion in 11/16 mtg