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.

Sign up for GitHub

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

Jump to bottom

Refactor of ActivityExpression and derived classes #1175

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Refactor of ActivityExpression and derived classes #1175

merged 2 commits into from
Oct 13, 2023

Conversation

adrienmaillard
Copy link
Contributor

  • Tickets addressed: None. Separated this commit from another PR on @Mythicaeda 's request.
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

The hierarchy was an unnecessary mess. The approach taken is: get rid of all but ActivityExpression, the parent class of all.

Verification

It's a refactor. All existing tests pass.

Documentation

None.

Future work

Same kind of approach for #1173

@adrienmaillard adrienmaillard added refactor A code change that neither fixes a bug nor adds a feature scheduling Anything related to the scheduling domain labels Oct 3, 2023
@adrienmaillard adrienmaillard requested a review from a team as a code owner October 3, 2023 00:26
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 3, 2023 00:26 — with GitHub Actions Inactive
Mythicaeda
Mythicaeda reviewed Oct 4, 2023
View reviewed changes
scheduler-driver/src/main/java/gov/nasa/jpl/aerie/scheduler/goals/CoexistenceGoal.java Outdated Show resolved Hide resolved
scheduler-driver/src/main/java/gov/nasa/jpl/aerie/scheduler/EquationSolvingAlgorithms.java Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
@adrienmaillard adrienmaillard force-pushed the refactor-ActivityExpression-hierarchy branch from 730f349 to eab1244 Compare October 11, 2023 20:16
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 11, 2023 20:16 — with GitHub Actions Inactive
@adrienmaillard
Copy link
Contributor Author

@Mythicaeda I have made the changes.
One notable thing: I had omitted to make one change in PrioritySolver prior to using its getLatestSimResultsUpTo in rootfinding (we had a very similar method in ActivityCreationTemplate...without synchronization). We don't want to synchronize the plan with sim when doing rootfinding so I had to pull that procedure. It's in the first commit.

@adrienmaillard adrienmaillard force-pushed the refactor-ActivityExpression-hierarchy branch from eab1244 to a45153b Compare October 12, 2023 23:09
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 12, 2023 23:09 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard force-pushed the refactor-ActivityExpression-hierarchy branch from a45153b to 9cbe43e Compare October 12, 2023 23:11
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 12, 2023 23:11 — with GitHub Actions Inactive
@adrienmaillard
Copy link
Contributor Author

Made the changes + I have collapsed the 2 matches method into one with a supplementary parameter commanding whether the match on arguments should be on the subset of total equality. Cleaner.

Mythicaeda
Mythicaeda approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@Mythicaeda Mythicaeda left a comment

Choose a reason for hiding this comment

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

LGTM! Last couple comments are minor code cleanup and can be ignored

...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Show resolved Hide resolved
...er/src/main/java/gov/nasa/jpl/aerie/scheduler/constraints/activities/ActivityExpression.java Outdated Show resolved Hide resolved
@adrienmaillard adrienmaillard force-pushed the refactor-ActivityExpression-hierarchy branch from 9cbe43e to 67bb88a Compare October 12, 2023 23:52
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 12, 2023 23:52 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard merged commit 4543e78 into develop Oct 13, 2023
6 checks passed
@adrienmaillard adrienmaillard deleted the refactor-ActivityExpression-hierarchy branch October 13, 2023 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mythicaeda Mythicaeda Mythicaeda approved these changes

@JoelCourtney JoelCourtney Awaiting requested review from JoelCourtney JoelCourtney is a code owner automatically assigned from NASA-AMMOS/aerie-backend

Assignees

@adrienmaillard adrienmaillard

Labels
refactor A code change that neither fixes a bug nor adds a feature scheduling Anything related to the scheduling domain
Projects
Aerie
Archived in project
Milestone
FY23 Q4 - Ad Hoc Improvements
Development

Successfully merging this pull request may close these issues.
3 participants
@adrienmaillard @Mythicaeda @mattdailis