interface files to simplify .mli files in owl/dense

[tachukao]

• Reduced .mli redundancy in owl/dense

Currently, there's a lot of redundancy in the .mli files in src/owl/dense. Adding a new ndarray function involves updating multiple .mli files. I've added two interface files (src/owl/owl_dense_ndarray_intf.ml and src/owl/owl_dense_matrix_intf.ml) to simplify this process.

In doing so, I've also discovered that we are exposing multiple functions that are not defined for complex ndarrays such as signum. This is likely a mistake due to copying and pasting the .mli files. The added interface files make sure that we separate signatures that are specific to real ndarrays from those that are specific to complex ndarrays.

I suspect something similar can be done for .mli files in src/owl/linalg and also other parts of the repo.

• applied ocamlformat to files in src/owl/dense

TODO: finish adding docstrings to the interface files

[ryanrhymes]

It is better to separate ocamlformat as a separate PR. First it is easier for code reviewing, to focus on core code and structural changes rather than formatting changes. Second, it is better to gradually adopt ocamlformat.

Dense is core part and has many lines of code, I personally think the format changes are a bit too drastic at the moment, also impacts the documentation a lot. For structural changes, I will take a deeper look later.

[tachukao]

I've reversed the effect of ocamlformat in the latest commits

[jzstark]

Agree. I think we should separate the change of format to another PR for ease of revision. Personally I don't find updating to multiple .mli files a big inconvenience, since that doesn't happen very frequently for now. I'm also not sure about if the interfaces of float ndarray should be the same with that of complex ndarray.

[tachukao]

@jzstark agreed, I've reversed the effect of ocamlformat. As for the interface, I've made it such that you have a Base interface which is shared among both complex and real Ndarrays, while interface Real is specific to real Ndarrays and Complex is specific to complex Ndarrays.

[ryanrhymes]

I like the changes overall. The new strucutre generalises the interfaces of Dense module very well and reduces the codebase significantly. Similar method was actually already adopted in CGraph and some other modules in Owl, I think this can/should be applied to Linalg as well to simplify Owl's core.

This certainly poses some challenges in Owl's document building process, but in general I think pros are over cons. Especially with the new tutorial book and gradual evolution of Owl's API doc, the impacts can be mitigated.

I am currently pushing hard on architectural retrospection on Owl's core components. Owl repo itself has now over 180k LOC (incl. c, c++, ocaml, etc.). Other sublibs atop of Owl are also growing fast as well. We need to keep the core concise (improve maintainability) without losing flexibility and functionality. I think this PR is a good attempt.

I included more reviewers so we are open to different opinions, we shall also discuss more on this kind of architectural improvement. Actually architectural discussion is on the TODO list in our meeting next year in Groningen.

[jzstark]

Thank Calvin. I think categorising existing ndarray signatures Base/Real/Complex/Distribution is quite neat. Besides the small concern that perhaps the base_ndarray should also change with it, I cannot think of a case where this change would lead to practical problems. The only thing is that there is this design issue about whether it is a proper time to do the generalisation/abstraction. I think it would be wise to hear Liang's opinion on the choice design.

[ryanrhymes]

The only thing is that there is this design issue about whether it is a proper time to do the generalisation/abstraction ...

Chatted with Jianxin a couple of minutes ago, he has some insightful comments. Both Base and Core have Dense module, and ideally they should be consistent in both functionality and design. Therefore, it may actually make (even more) sense to move *_intf.ml files to Base lib since they are pure OCaml anyway and both Core and Base can use them.

The issue is due to the limited recources, the Dense module in Base only implements a limited set of APIs comparing to its corresponding one in Core. Using these *_inft.ml file probably means we need to create a lot of dummy stubs and implement these functions in future. However, if we keep the Dense in Base as it is now, Dense in Base and Core will have inconsistent design. I am myself
not 100% sure whether it is a big issue or not.

[tachukao]

The issue is due to the limited recources, the Dense module in Base only implements a limited set of APIs comparing to its corresponding one in Core. Using these *_inft.ml file probably means we need to create a lot of dummy stubs and implement these functions in future

Yea I thought about implementing the *_intf.ml files in src/base/dense, but gave up on the idea in the end precisely because of this. Adding dummy stubs is a bit tedious, but I guess it's also a good way of documenting todo...but yea it's going to be quite a lot of work...

[mseri]

This is a welcome change! I have some comments and will not be able to write them down and do a careful review before tomorrow or Tuesday morning. It would be great if you can wait until then before the merge

[ryanrhymes]

This is a welcome change! I have some comments and will not be able to write them down and do a careful review before tomorrow or Tuesday morning. It would be great if you can wait until then before the merge

A sure thing, this is not super urgent, merge can wait.

[ryanrhymes]

The issue is due to the limited recources, the Dense module in Base only implements a limited set of APIs comparing to its corresponding one in Core. Using these *_inft.ml file probably means we need to create a lot of dummy stubs and implement these functions in future

Yea I thought about implementing the *_intf.ml files in src/base/dense, but gave up on the idea in the end precisely because of this. Adding dummy stubs is a bit tedious, but I guess it's also a good way of documenting todo...but yea it's going to be quite a lot of work...

No problem, I am happy with current PR. We can create another PR for migrating this to Base. Yes there will be some tedious work to make dummy stubs in Base to make it consistent with the new design. But I am myself happy to take that task :D

[tachukao]

In the latest commit, I did the same thing for owl/linalg as discussed above

[mseri]

I have had a look at the code. Thanks again for the cleanup.

My only remaining comment is that one of the most important features of Owl is that it reduces by a great margin the failures to compile time, having the full interface in Base with lots of not implemented stubs will, imho, make it much worse.

I think a middle ground would be the best: having a base interface as large as possible with the implemented functions, and a core interface (exposed only in owl-core) with what is missing. Then we can slowly work to reduce the core interface and extend the base one, without breaking existing code and making sure that whatever ends up in base, ends up there with an implementation.

What do you think about it?

[jzstark]

Thanks for the update Calvin! I think in the end we should move the signatures to Base and get unified between core and base, and Marcello's incremental solution seems quite nice. E.g in Linear algebra module, a lot of routines are to be implemented, and there is also some mismatch between base ndarray and core ndarray, such as the to_array function, so updating the signature one by one looks like a good choice, though I'm not quite clear et how such a transitional solution should be implemented and cope with existing base signatures.

Btw, sorry that I forgot to mention it, but here is the style suggestion I asked about making space between one-line functions in ocamlformat, and it turns out that adding module-item-spacing=sparse in the configuration file would work nicely.

[mseri]

though I'm not quite clear et how such a transitional solution should be implemented and cope with existing base signatures.

Core signature will include base siugnatures, so the transition will simply require take us moving the definitions of the signature from one interface to the other and add the necessary definitions in base ml files.

[mseri]

and there is also some mismatch between base ndarray and core ndarray, such as the to_array function

Indeed, these are annoying and will be fixed once the signatures are unified

[mseri]

Btw, sorry that I forgot to mention it, but here is the style suggestion I asked about making space between one-line functions in ocamlformat, and it turns out that adding module-item-spacing=sparse in the configuration file would work nicely.

🎉

[tachukao]

@mseri I like that solution. Should we merge this PR for now and I can get cracking on migrating the interface to base? Or do you think it's best to do it all in this PR?

@jzstark RE ocamlformat. That's great! perhaps we should leave the formatting to a second PR as well?

[mseri]

@mseri I like that solution. Should we merge this PR for now and I can get cracking on migrating the interface to base? Or do you think it's best to do it all in this PR?

This seems a good idea to me. Doing it all in this PR would be overkill

[ryanrhymes]

module-item-spacing=sparse is already in the ocamlformat, added a couple of days ago. :)

[ryanrhymes]

fyi, the PR reduces about 2.1K LOC, great job!