Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #17 #77

Closed
wants to merge 7 commits into from
Closed

Closes #17 #77

wants to merge 7 commits into from

Conversation

alfoa
Copy link
Collaborator

@alfoa alfoa commented Oct 10, 2023


Pull Request Description

What issue does this change request address?

Closes #17

What are the significant changes in functionality due to this change request?

This simple change allows the user to set construction times > 1 unit (year, month, etc) in the Capex cash flow calculation.
In the user manual, the alpha can already be set as an array but if the "driver" is a variable (for example, from raven) only the first "year" entry is taken in consideration. This MR fixes that. No manual modifications have been performed since the manual seems to indicate that this is already possible (even if it is not).


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large tes.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added, the the analytic documentation must be updated/added.
  • 9. If any test used as a basis for documentation examples have been changed, the associated documentation must be reviewed and assured the text matches the example.

dylanjm
dylanjm previously approved these changes Oct 11, 2023
Copy link
Collaborator

@dylanjm dylanjm left a comment

Choose a reason for hiding this comment

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

Changes LGTM and will satisfy a long-standing feature request.

@dylanjm
Copy link
Collaborator

dylanjm commented Oct 11, 2023

@alfoa Looks like there is some trailing whitespace in Cashflows.py

src/Amortization.py Outdated Show resolved Hide resolved
src/Amortization.py Outdated Show resolved Hide resolved
src/Amortization.py Outdated Show resolved Hide resolved
@alfoa
Copy link
Collaborator Author

alfoa commented Oct 11, 2023

@dylanjm @worseliz can you check if it makes sense on your side as well?(Maybe with a simple excel case?)?

@alfoa
Copy link
Collaborator Author

alfoa commented Oct 11, 2023

@dylanjm @worseliz Pyomo test is failing. Do you think it is related?

@dylanjm
Copy link
Collaborator

dylanjm commented Oct 12, 2023

@alfoa Yes, it looks like it's calculating a different NPV for the PyomoTest:

image

I will take a look at it deeper in a little bit. Looks like the new NPV calculation is off by about $840

@alfoa
Copy link
Collaborator Author

alfoa commented Nov 27, 2023

@dylanjm @worseliz did you have a chance to take a look at this?

@alfoa alfoa requested a review from dylanjm November 28, 2023 20:30
@moosebuild
Copy link

Job CentOS 8 on 6af5cce : invalidated by @dylanjm

Rerun

@dylanjm
Copy link
Collaborator

dylanjm commented Nov 29, 2023

@alfoa I'm able to recreate the PyomoTest failure on my local machine. It appears PyomoTest is passing on the github workflow machines because it is missing ipopt and skipping the PyomoTest When I switch back to the main branch, I do not get these errors.

There is probably something messed up in PyomoTest.py that is assuming something incorrect about the capex cashflow construction that needs to be changed.

@alfoa
Copy link
Collaborator Author

alfoa commented Dec 1, 2023

@alfoa I'm able to recreate the PyomoTest failure on my local machine. It appears PyomoTest is passing on the github workflow machines because it is missing ipopt and skipping the PyomoTest When I switch back to the main branch, I do not get these errors.

There is probably something messed up in PyomoTest.py that is assuming something incorrect about the capex cashflow construction that needs to be changed.

Hi @dylanjm ,
How de want to approach this?

@yoshiurr-INL it seems you created the test. Advices?

@alfoa
Copy link
Collaborator Author

alfoa commented Jan 2, 2024

@alfoa
Copy link
Collaborator Author

alfoa commented Jan 3, 2024

I close this PR. I ll keep the changes local. If somebody wants to pick this up, the branch still exists in the repository.
Thanks

@alfoa alfoa closed this Jan 3, 2024
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.

[TASK] More Time Flexibility with CAPEX
3 participants