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

Use Match-Replace strategy to alias operators #37

Merged
merged 7 commits into from
Jun 26, 2024
Merged

Conversation

lukem12345
Copy link
Member

@lukem12345 lukem12345 commented Jun 7, 2024

Close #35

As a compromise between:

  • considerations of computational efficiency
  • warts relating to the types of "matched" variables destroying "context"
    , a "match & replace" strategy is adopted - inspired by AlgebraicRewriting - but using tight loops tailored to our workflow.

@lukem12345 lukem12345 added the enhancement New feature or request label Jun 7, 2024
@lukem12345 lukem12345 self-assigned this Jun 7, 2024
@lukem12345 lukem12345 requested a review from jpfairbanks June 7, 2024 20:27
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 79.41176% with 21 lines in your changes missing coverage. Please review.

Project coverage is 86.62%. Comparing base (261b0fb) to head (33630d6).

Files Patch % Lines
src/openoperators.jl 80.19% 20 Missing ⚠️
src/acset.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
+ Coverage   85.15%   86.62%   +1.47%     
==========================================
  Files          12       13       +1     
  Lines         788      890     +102     
==========================================
+ Hits          671      771     +100     
- Misses        117      119       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lukem12345 lukem12345 requested review from jpfairbanks and removed request for jpfairbanks June 10, 2024 18:15
@lukem12345
Copy link
Member Author

I'll stick a pin in 03aa7ce as a complete version of this feature in an imperative/ database-manipulation manner.

I'll continue commits to this branch from the operadic composition approach.

@jpfairbanks
Copy link
Member

Let's keep the other approach in separate files so that we can compare and contrast the changes. I'm thinking the repurposing of oapply will be much more maintainable long term.

@lukem12345
Copy link
Member Author

Record of discussion with @GeorgeR227 :

  • Take care that the RHS Decapodes are fully expanded
  • Reformat (use newlines) in replace-operator functions
  • Merge the "more imperative" code (after review process) in 03aa7ce. (It is more declarative than existing code currently in Decapodes.)
  • Decide whether we should export the single-operator replacement functions.
  • Even the oapply approach will need some imperativity on top of it, either creating RHS Decapodes on-the-fly with the names of the src/tgt variables, or in the LHS, and the replaced operator still has to be explicitly removed.

@lukem12345
Copy link
Member Author

lukem12345 commented Jun 24, 2024

Default composition code has been cherry picked onto PR #52 . Upon that merging, I will reset this PR to 03aa7ce

@lukem12345 lukem12345 merged commit e21e6c7 into main Jun 26, 2024
10 checks passed
@lukem12345 lukem12345 deleted the llm/open-ops branch June 26, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better support for using Operator Aliases
2 participants