Skip to content

Conversation

@cpaulgilman
Copy link
Collaborator

@cpaulgilman cpaulgilman commented Aug 9, 2024

Fix #1837

  • Check formulas that refer to named ranges like "ibi_fed_percent_deprbas_sta" to make sure they use correct fed, sta, uti, oth prefix or suffix.
  • Check series of formulas that use incrementing cell references.

Go through all incentive calculations in Cash Flow workbook for:

  • State income tax
  • Federal income tax
  • State depreciation and ITC basis table
  • Federal depreciation and ITC basis table
  • State depreciable basis incentive reductions
  • Federal depreciable basis incentive reductions

Check box in table indicates item was fixed, "ok" indicates item did not need to be fixed.

Issue TPO SO CS PF SLB MP
State income tax IBI as percentage named ranges all refer to fed prefix ok ok ok ok ok
State income tax CBI named ranges all refer to fed prefix ok ok ok ok ok
Federal income tax IBI as percentage do not refer to taxable/not taxable choice on Inputs sheet ok ok
Federal income tax CBI do not refer to taxable/not taxable choice on Inputs sheet ok ok ok ok ok
Federal income tax CBI refers to sta suffix instead of fed ok ok ok ok ok
State depreciation/ITC ITC qualifying costs refers to wrong cell ok
State depreciation/ITC ITC reduction federal refers to wrong cell ok ok ok ok ok
Federal depreciation/ITC first year bonus depreciation cell references instead of named ranges ok ok ok ok ok

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 financial Financial model UI User interface issue that applies across performance and financial models labels Aug 9, 2024
@cpaulgilman cpaulgilman added this to the 2023 Release Patch 2 milestone Aug 9, 2024
@cpaulgilman cpaulgilman requested a review from brtietz August 9, 2024 16:23
@cpaulgilman cpaulgilman self-assigned this Aug 9, 2024
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.

This looks great, thank you!

Do you have thoughts on how to do the merge to develop? Given that these are binary files I'm concerned that you'll have to do this work twice, I'd love to hear if you have any ideas on clever workarounds.

@cpaulgilman
Copy link
Collaborator Author

Do you have thoughts on how to do the merge to develop? Given that these are binary files I'm concerned that you'll have to do this work twice, I'd love to hear if you have any ideas on clever workarounds.

The most foolproof method would be to manually replace the .xlsx files in Develop with ones from Patch. That could be done as part of the effort to merge Patch into Develop after we release Patch 2, or as a separate step after the merge.

@cpaulgilman cpaulgilman merged commit 0612373 into patch Aug 9, 2024
@cpaulgilman cpaulgilman deleted the send-to-excel-ibi-fixes branch August 9, 2024 19:07
@brtietz
Copy link
Collaborator

brtietz commented Aug 9, 2024

Do you have thoughts on how to do the merge to develop? Given that these are binary files I'm concerned that you'll have to do this work twice, I'd love to hear if you have any ideas on clever workarounds.

The most foolproof method would be to manually replace the .xlsx files in Develop with ones from Patch. That could be done as part of the effort to merge Patch into Develop after we release Patch 2, or as a separate step after the merge.

That's a great way to ensure these issues get cleaned up, but I'm concerned that will stomp on the existing changes related to #1803. Which set is easier and safer to redo?

@cpaulgilman cpaulgilman restored the send-to-excel-ibi-fixes branch August 9, 2024 20:20
@cpaulgilman
Copy link
Collaborator Author

Do you have thoughts on how to do the merge to develop? Given that these are binary files I'm concerned that you'll have to do this work twice, I'd love to hear if you have any ideas on clever workarounds.

The most foolproof method would be to manually replace the .xlsx files in Develop with ones from Patch. That could be done as part of the effort to merge Patch into Develop after we release Patch 2, or as a separate step after the merge.

That's a great way to ensure these issues get cleaned up, but I'm concerned that will stomp on the existing changes related to #1803. Which set is easier and safer to redo?

Good point. The easiest is to redo the updates to the basis prior to allocation calculations from https://github.com/NREL/SAM/tree/sam_1803_basis_correction. (Should that branch be off of Develop instead of Patch?) I think we (I) should copy the workbooks from Patch into that branch, and then redo the basis calcs.

@brtietz
Copy link
Collaborator

brtietz commented Aug 9, 2024

Yes, that branch (and whatever branch we use going forward for these changes) should be off of develop.

Moving branches or commits between patch and develop is a little tricky right now due to NREL/ssc#1201. I think the right process is:

  1. Wait for the team to finish ssc 1201
  2. Merge patch into develop one last time
  3. Make the changes you describe above.

@cpaulgilman cpaulgilman added the added to release notes PR and/or issue has been added to release notes for a public release label Aug 23, 2024
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 bug financial Financial model UI User interface issue that applies across performance and financial models

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Additional send to excel changes for IBI

3 participants