math-comp / algebra-tactics

Ring, field, lra, nra, and psatz tactics for Mathematical Components
32 stars 2 forks source link

Port to Hierarchy Builder #71

Closed proux01 closed 1 year ago

proux01 commented 1 year ago

C.f. https://github.com/math-comp/math-comp/pull/733

pi8027 commented 1 year ago

zmodule.v is the running example for the paper "Reflexive tactics for algebra, revisited". So please try to keep the same structure as much as possible and do not rename things, e.g., MMorph. (It is also acceptable to just remove it for the moment, since it is available on Zenodo anyway.)

proux01 commented 1 year ago

I thought you implemented what I suggested, but that was not true. So never mind. I pushed my fix.

No, sorry, I didn't catch all the details of what you wanted to do.

Thanks for the code! I had a careful look and it looks good to me. I think that, modulo the discussion about all the *__canonical__* (without forgetting GRing_Zmodule__to__GRing_Nmodule in common.v that is only used in zmodule.v), it can be squashed, merged and released.

I still feel there is a massive amount of duplication in all this code though. But fixing that would probably require some major refactoring and definitely doesn't belong to the current PR.

zmodule.v is the running example for the paper [...]

Thanks for the explanation.

pi8027 commented 1 year ago

Regarding the "duplication" between the reification procedures for field and lra, the reason to duplicate it is not only performance. For me, the preprocessors required for these two tactics are different enough to implement two different reification procedures. Also, the preprocessors are becoming more and more difficult to maintain anyway. I believe that we should eventually generate the reification procedures automatically to address this maintainability issue (which is really a long-term goal).

proux01 commented 1 year ago

I'm not sure I understand what you mean by "automatically generating the reification procedure"? I wonder whether we shouldn't completely change the parsing model with more intermediate languages, a reification procedure that would perform less work and could be shared, then a first simplification pass to a simpler intermediate language, finally the normalization procedures specific to each tactic (that would have to modify the variable map VM). But that's not a very precise idea and I agree about the "long-term goal" part.

pi8027 commented 1 year ago

The functionality I need would be similar to the quote tactic bundled in some older versions of Coq. But, instead of just taking the interpretation function and performing reification, I'm thinking about translating the interpretation function to a set of Elpi rules that performs reification. This is not sufficient in our case since we have two kinds of reified terms, but I have another vague idea to overcome this difficulty.

pi8027 commented 1 year ago

Apparently, coq.elpi.accumulate-clauses is available only from Coq-Elpi 1.18.0 that requires Coq 8.17. Is it ok to drop the support for Coq 8.16, or should we use coq.elpi.accumulate instead?

proux01 commented 1 year ago

Apparently, coq.elpi.accumulate-clauses is available only from Coq-Elpi 1.18.0 that requires Coq 8.17. Is it ok to drop the support for Coq 8.16, or should we use coq.elpi.accumulate instead?

Since MC 2.0.0 supports 8.16, it would probably be nice to keep supporting it. Considering the number of clauses, the performance impact of using accumulate instead of accumulate-clauses is likely negligible.

gares commented 1 year ago

If you are not accumulating thousands of clauses, the new API provides no gain. In HB we accumulate like 60k clauses, grouping them changes the perf. If you are accumulating 100, you don't need the new API.

pi8027 commented 1 year ago

Ok.

Considering the number of clauses, the performance impact of using accumulate instead of accumulate-clauses is likely negligible.

Also, the accumulation happens only when compiling Algebra Tactics. So I would say it is irrelevant to the performance from the user's perspective.

proux01 commented 1 year ago

To be precise, it's also about the Elpi Accumulate Db canonicals.db. that happens at each tactic call. But again, considering the number of clauses, not an issue here.

pi8027 commented 1 year ago

Shall we merge? Feel free to squash a bit but I'm not going to do that by myself.

proux01 commented 1 year ago

Shall we merge?

Sure 🎉

Feel free to squash a bit

done

proux01 commented 1 year ago

@pi8027 do you plan to do a release? (I can handle the OPAM and Nix packages)

pi8027 commented 1 year ago

@proux01 I'm waiting for the green light here: coq-community/apery#16. Meanwhile, I drafted the release (I think you have access).