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

Extend expression_manipulations.jl to some variables #579

Merged
merged 6 commits into from
Feb 1, 2024

Conversation

RuaridhMacd
Copy link
Collaborator

As pointed out in PR #562 and elsewhere, there are still some issues with the expression_manipulations functions. This PR tries to resolve the following:

  1. There are some undefined functions, e.g. _sum_expressions()
  2. There are some unnecessary duplicate methods with concrete instead of abstract types
  3. Some bugs exist around edge-cases with various combinations of types
  4. Some of the methods don't work with variables when it seems they should, e.g. add_similar_to_expression()
  5. The methods aren't documented.
  6. The sum_expression() methods don't have tests

@cfe316 has addressed issue number 4 in PR #562 but I also included a quick way of handling variables using the 1 * dummy coefficient that he proposed.

There are still some extensions we could make, in particular creating arrays using generic expressions and variables which don't use Float64 coefficients and constants or that have non-dense indices.

The SmallNewEngland cases worked for me with these changes but I'd definitely appreciate help checking for any edge cases.

- Added docstrings. Still need to add to the documentation
- Simplified the versions further
- Added add_to_expression for variables, using Jacob's 1 * VarRef trick
@RuaridhMacd RuaridhMacd self-assigned this Nov 14, 2023
@RuaridhMacd RuaridhMacd added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 14, 2023
@RuaridhMacd RuaridhMacd added this to the v0.4 milestone Nov 14, 2023
@RuaridhMacd
Copy link
Collaborator Author

One note: I simplified some of the indexing using eachindex(). This sacrifices a small amount of speed / memory usage but makes the methods work for AbstractArrays with sparse indices and reduces the number of functions we need to maintain.

@RuaridhMacd RuaridhMacd marked this pull request as draft November 14, 2023 22:19
@RuaridhMacd
Copy link
Collaborator Author

Converting this to a draft as there's an issue with the methods which take GenericVariableRef. The methods work but then don't save the new expression in place of the variable. I'll look at a fix.

We can't treat Variables and Expressions the same if we want all the functions to be in-place. Using a variable will work but we can't reassign the new expression to the original variable. We could consider other helper functions which take in variables and turn them into expressions. However, in many cases it'll be easier to use add_similar_to_expression!() with the variable as the second argument
@RuaridhMacd RuaridhMacd marked this pull request as ready for review January 18, 2024 15:17
@RuaridhMacd
Copy link
Collaborator Author

I've removed the functions which worked on variables, rather than expressions. They don't work as in-place functions. We can perhaps add new functions in the future to help users go from variables -> expressions

@RuaridhMacd
Copy link
Collaborator Author

@lbonaldo @sambuddhac I think this should be ok for v0.4

Copy link
Collaborator

@lbonaldo lbonaldo left a comment

Choose a reason for hiding this comment

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

Thanks @RuaridhMacd for this PR. I just added a couple of small comments.

@lbonaldo lbonaldo merged commit 0cb5f17 into develop Feb 1, 2024
9 checks passed
@RuaridhMacd RuaridhMacd deleted the expression_manip_tests_and_cleanup branch March 27, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants