AlgebraicJulia / DiagrammaticEquations.jl

MIT License
12 stars 1 forks source link

Safer type modification #47

Closed GeorgeR227 closed 5 months ago

GeorgeR227 commented 5 months ago

This should help solve problems in type inference regarding typing rules doing strange things. Addresses problems but does not fully resolve issue #46.

GeorgeR227 commented 5 months ago

To be clear, this should resolve the issues present in that issue but a MWE and proper tests are still not available.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.62%. Comparing base (b2acbe7) to head (6995d38).

Files Patch % Lines
src/acset.jl 93.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #47 +/- ## ========================================== + Coverage 84.58% 84.62% +0.04% ========================================== Files 12 12 Lines 759 761 +2 ========================================== + Hits 642 644 +2 Misses 117 117 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lukem12345 commented 5 months ago

I would change it such that safe_modifytype is mutating, and just returns true or false, unless its too unseemly to pass the necessary ref.

Adding the Halfar equation with avg_01 ought to suffice for now, and the code cov report is an automatic check to confirm that this is getting hit.

GeorgeR227 commented 5 months ago

I decided to make a modifying wrapper around the function. Reason being that I want the core of the function itself to be very short and concise, plus leaving it like this makes testing it easier since I don't need to build up Decapodes for it.

lukem12345 commented 5 months ago

Want to merge this now and do a separate PR for Constant?

GeorgeR227 commented 5 months ago

Yeah that's the plan. The Constant behavior is baked into the type inference so changing that seems more involved.

GeorgeR227 commented 5 months ago

Close #46. We now have a MWE and a test to cover it.