Delete Jkind_axis.Modal in favor of Mode.Alloc.axis #3409
Conversation
glittershark
force-pushed
the
aspsmith/with-kind-modalities
branch
from
e37ec6f to
84008ef
Compare
glittershark
force-pushed
the
aspsmith/normalize-mode-axis-types
branch
3 times, most recently
from
4c2935f to
f2abf37
Compare
glittershark
force-pushed
the
aspsmith/with-kind-modalities
branch
from
737a2a8 to
c01dc5d
Compare
glittershark
force-pushed
the
aspsmith/normalize-mode-axis-types
branch
2 times, most recently
from
3b9194f to
cb9f1f9
Compare
glittershark
force-pushed
the
aspsmith/normalize-mode-axis-types
branch
2 times, most recently
from
7f571bb to
e8ad948
Compare
glittershark
force-pushed
the
aspsmith/with-kind-modalities
branch
from
a3a71dc to
9354afe
Compare
glittershark
force-pushed
the
aspsmith/normalize-mode-axis-types
branch
from
e8ad948 to
ffeb5de
Compare
glittershark
force-pushed
the
aspsmith/with-kind-modalities
branch
from
9354afe to
7da1766
Compare
Per zqian's CR to this effect. note that this does not do any of the simplification that this unlocks yet, that'll happen later
Now that the types for jkind axes and mode axes have been unified, we can express the condition for "modality is const for axis" directly, without resorting to a huge ugly pattern match on the axis and the modality's atoms.
glittershark
force-pushed
the
aspsmith/normalize-mode-axis-types
branch
from
ffeb5de to
6ab8dcd
Compare
|
I’m currently finishing off a patch that completely rewrites this whole area. If this is blocking you then feel free to go ahead with this change, but if you are just tidying things up then it is probably better to wait for my upcoming PR. I’m out for another week and a bit busy, so it’s probably still 3 weeks away or so. |
|
@lpw25 Are you refering to the modality refactor? (adding a record type containing a modality for each axis; Are there other things as well?). |
|
The modality refactor is the first in a series of patches, the second of which rewrites all the Jaxis stuff. |
|
Is that not also going to generally conflict pretty badly with with-kinds? If so it'd be nice if you can hold off, since with-kinds landing is a pretty big blocker for us. |
There was a problem hiding this comment.
Reviewed.
Ideally, I think there are some further opportunity for code removal, I put comments at several places that are easy to do.
Mostly, I think jkind*.ml should avoid mentioning specific mode axes, and instead delegate to mode.ml. Specifically, lots of the pattern match on mode axes are already done in mode.ml.
Does @lpw25 's incoming patch already disentangle mode axes from jkind? If so, then maybe it's not worthwhile to do that in this PR, since that would create more conflicts.
This is a little jkind-specific to be polluting Mode with
glittershark
force-pushed
the
aspsmith/normalize-mode-axis-types
branch
from
5d41237 to
28e90a4
Compare
There was a problem hiding this comment.
Thanks for the changes - look good!
WRT the CI error - I think it's fine to just change Axis.print so it prints locality for Areality. Areality is our internal name to share between locality and regionality, which is not supposed to be seen by the users.
I think further code removal can be done incrementally in future PRs, we can merge this PR as it is.
@lpw25 Maybe instead of a record type containing a modality for each axis, we can have Mode.Value.axis_packed list containing all the axis, and have Mode.Modality.Value.Const.proj that projects an atom modality out of a whole modality? (I'm obsessed with the idea of not mentioning mode axes names in jkind.ml)
|
@glittershark Hmm I just noticed this PR merged into your branch - I think it's fine to merge into |
ah, yeah, this is targeting my branch because it also includes a refactor to a function introduced by that branch. I can cherry-pick only the relevant bits onto a new PR targeting |
Per zqian's CR to this effect. Also, this simplifies
modality_is_const_for_axis, introduced in the previous PR, to more directly express the property rather than relying on a big ugly pattern match.See the individual commits for more.