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

Patch bound constructors #85

Merged
merged 18 commits into from
Jan 14, 2025
Merged

Conversation

stelmo
Copy link
Member

@stelmo stelmo commented Jan 9, 2025

No description provided.

@stelmo stelmo force-pushed the sew-patch-constraint-constructor-enzymes branch from ffedb56 to e4f53a7 Compare January 9, 2025 09:44
src/builders/enzymes.jl Outdated Show resolved Hide resolved
@exaexa
Copy link
Member

exaexa commented Jan 9, 2025

Actually when thinking about it, this should not be patched in the enzymes function (this one just forwards the stuff transparently, which is all OK), but in the code that uses it. In particular, one should do e.g.

enzyme_constraints(
    ...,
    capacity_limits = [(:something, somefluxes, BetweenT{MyWeirdType}(0, somevariable))]
)

@stelmo
Copy link
Member Author

stelmo commented Jan 9, 2025

/format

@stelmo stelmo requested a review from exaexa January 9, 2025 15:35
triggered by @stelmo on PR #85
Copy link
Contributor

github-actions bot commented Jan 9, 2025

✔️ Auto-formatting triggered by this comment succeeded, commited as a1d8658

src/frontend/enzymes.jl Outdated Show resolved Hide resolved
@exaexa exaexa marked this pull request as draft January 13, 2025 15:35
@exaexa
Copy link
Member

exaexa commented Jan 14, 2025

@stelmo I decided to do this properly, pls review

Might be the case that I need to add some tests still, will do later

@exaexa exaexa marked this pull request as ready for review January 14, 2025 09:05
@exaexa exaexa force-pushed the sew-patch-constraint-constructor-enzymes branch from 4f7d366 to 1ea118f Compare January 14, 2025 09:07
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d6004be) to head (9be3961).

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #85   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           38        38           
  Lines          573       579    +6     
=========================================
+ Hits           573       579    +6     

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

Copy link
Member

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

hopefully ok now, need to fix the tests

@exaexa exaexa marked this pull request as draft January 14, 2025 09:54
@exaexa
Copy link
Member

exaexa commented Jan 14, 2025

The last 3 hours wasted on this simple issue prove that we need an actual type system.

@exaexa exaexa marked this pull request as ready for review January 14, 2025 10:44
Copy link
Member

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

oh finally 🤯

Copy link
Member Author

@stelmo stelmo 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 good to me. release 🦀 ?

@exaexa exaexa merged commit eefa8d3 into master Jan 14, 2025
4 checks passed
@exaexa exaexa deleted the sew-patch-constraint-constructor-enzymes branch January 14, 2025 13:10
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.

3 participants