Skip to content

Conversation

@brtietz
Copy link
Collaborator

@brtietz brtietz commented Sep 22, 2021

Convert two functions currently in pysam's battery tools to ssc equations such that they can be used by the API in all languages.

Future pull requests will add additional cmod_battery functions.

…ions such that they can be used by the API in all languages
@brtietz brtietz added this to the SAM Fall 2021 Release milestone Sep 22, 2021
@brtietz brtietz requested a review from dguittet September 22, 2021 21:06
vt_get_number(vt, "original_capacity", &original_capacity);
vt_get_number(vt, "desired_capacity", &desired_capacity);

double mass_per_specific_energy = mass / original_capacity;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is an edge case where the original_capacity is 0 and then resized to non-zero. This happens when you size the battery to 0, perhaps as part of a loop across a range of numbers, then the mass and surface area will get set to 0 also. Then for later sizing calls, the function will no longer be able to use prior information about the mass to calculate an appropriate mass, and here it'll cause a divide by 0.

Other cmods are not able to run with a strictly 0 system capacity (pvsamv1, windpower) while others can (pvwatts). While this is a still-unfixed issue in the PySAM code, I think the simplest way to handle this is to not let the desired_capacity be 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point! In the latest commit I used the same error convention as cmod_windpower_eqns - writing the error to an "error" variable in the vartable. Am I correct to assume it's the responsibility of the calling code to check this value? When I tried to throw an exec error, it crashed Julia. Given the difficulty of error handling in the SDK, I assume that's not a valid option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, you can't throw exceptions across a DLL boundary so that's not possible. The error will have to be done via a return string. Perhaps we could change the function signature of SSC equations so the return type is bool instead of void. I don't think that would be too difficult and would make error handling easier

Copy link
Collaborator

@dguittet dguittet Oct 7, 2021

Choose a reason for hiding this comment

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

Let me know if you want me to change the function signature to bool. Otherwise you have to check if error len > 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be super helpful. I tried to do the update just now and ran into a type issue:

Severity Code Description Project File Line Suppression State
Error C2440 'initializing': cannot convert from 'bool (__cdecl *)(ssc_data_t)' to 'ssc_equation_ptr' (compiling source file C:\Users\bmirletz\source\repos\sam_dev\ssc\ssc\core.cpp) ssc C:\Users\bmirletz\source\repos\sam_dev\ssc\ssc\ssc_equations.h 87

It looks like we either need to change them all or define a new signature with bool. I changed the battery stateful equations in the most recent push to get that error, if you're able to do the others I'd appreciate it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies for the delay, but I've fixed up all the equations: changing the return type, fixing up the error handling from exceptions to error strings.

@dguittet
Copy link
Collaborator

I'll re-review once tests are done

@brtietz brtietz merged commit 3b6456c into develop Oct 20, 2021
@brtietz brtietz deleted the move_stateful_sizing_to_ssc_eqns branch November 5, 2021 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants