Skip to content

Conversation

@sjanzou
Copy link
Collaborator

@sjanzou sjanzou commented Feb 22, 2021

No description provided.

Copy link
Collaborator

@brtietz brtietz left a comment

Choose a reason for hiding this comment

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

I checked out the changes and tested locally and this looks great! Obviously needs to wait for the SSC PR to be ready (see comments there).

Copy link
Collaborator

@cpaulgilman cpaulgilman left a comment

Choose a reason for hiding this comment

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

The total number of cells in now positive, and close to correct, but the value shown for 14 cells and 158,730,160 strings is 2,222,222,336 and, if my calculator is working correctly, I would expect it to be 2,222,222,240.

image

@sjanzou
Copy link
Collaborator Author

sjanzou commented Feb 23, 2021

@cpaulgilman, another fun experiment in rounding... In Excel, multiply 14 by 158730160 and get 2222222240 as your correct calculator showed. Now, take 2222222236 and divide by 158730160 and get 14 in Excel (see attached workbook.
Based on your observation and comments from @brtietz and @dguittet, I suggest we move this issue to Patch 2 or update all constraints to have max integer (2147483647) for Patch 1 as suggested by @dguittet for consistency between UI and all libraries and compute modules in ssc

@sjanzou
Copy link
Collaborator Author

sjanzou commented Feb 23, 2021

Rounding error as reported by Excel:
Rounding error.xlsx

@sjanzou
Copy link
Collaborator Author

sjanzou commented Feb 23, 2021

The issue from @cpaulgilman demonstrated in Excel
image

@sjanzou sjanzou self-assigned this Apr 21, 2021
@sjanzou
Copy link
Collaborator Author

sjanzou commented Apr 21, 2021

@cpaulgilman, @brtietz, @dguittet are we good for patch 2 with PR #543 ?

@brtietz
Copy link
Collaborator

brtietz commented Apr 21, 2021

@sjanzou Looks like there's still a failing test in NREL/ssc#541, and I'm not sure the discussion with @dguittet there was fully resolved? Something to discuss during next week's meeting? Or a side meeting?

@brtietz
Copy link
Collaborator

brtietz commented Nov 16, 2021

Closed as per discussion in 11/16 mtg

@brtietz brtietz closed this Nov 16, 2021
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.

4 participants