Remove the Model abstract type, simplify model migration code.
#172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typecon too allow subtyping in WithModel Change how import works. remove redundant Using Gatlab . . fix docstrings .
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## withmodel_subtype #172 +/- ##
=====================================================
- Coverage 95.56% 95.43% -0.14%
=====================================================
Files 38 38
Lines 2234 2257 +23
=====================================================
+ Hits 2135 2154 +19
- Misses 99 103 +4 ☔ View full report in Codecov by Sentry. |
kris-brown
changed the title
Model abstract type, simply model migration code.Model abstract type, simplify model migration code.
jpfairbanks
approved these changes
Dec 11, 2024
| @@ -75,20 +71,26 @@ struct ImplementationNotes | |||
| end | |||
|
|
|||
| """ | |||
| `implements(m::Model, tag::ScopeTag) -> Union{ImplementationNotes, Nothing}` | |||
| `implements(m::MyModel, tag::ScopeTag) -> Union{ImplementationNotes, Nothing}` | |||
There was a problem hiding this comment.
shouldn't this be on the first line indented by 4 spaces to make it monospace?
|
|
||
| using MLStyle | ||
| using DataStructures: DefaultDict, OrderedDict | ||
|
|
There was a problem hiding this comment.
ok, this is just moved from above.
|
|
||
| implements(m::Model, theory_module::Module) = | ||
| impl_type(m, mod::Module, name::Symbol) = | ||
| impl_type(m, gettag(ident(mod.Meta.theory; name))) |
There was a problem hiding this comment.
Remind me what the ;name without a kwarg does?
| @@ -9,13 +9,15 @@ using StructEquality | |||
| hom::HomT | |||
| end | |||
|
|
|||
| @struct_hash_equal struct SliceC{ObT, HomT, C<:Model{Tuple{ObT, HomT}}} <: Model{Tuple{SliceOb{ObT, HomT}, HomT}} | |||
| @struct_hash_equal struct SliceC{ObT, HomT, C} | |||
There was a problem hiding this comment.
I like this massive reduction in type constraining code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the
Modelabstract type which was overly constraining (preventing one and the samestructfrom being a model of many different theories). One purpose theModeltype served was to make it easy to see what Julia types were associated with the theory type constructors, but this functionality is now provided byimpl_typemethods which get defined by@instance.This also led to an opportunity to simply model migration code, which is no longer handled convolutedly within the module of a
@theorymapbut instead is a function (which callsevalto create a new struct which just wraps the input model). If we start actually using model migrations and get worried about generating too many structs, it wouldn't be hard to cache the inputs to this function.All interesting changes are in
ModelInterface.jl. Lots of tiny changes (mostly removing<: Model{...}are in other files.