ziotom78 / Ducc0.jl

Julia bindings to ducc0
MIT License
1 stars 0 forks source link

Updated interface #1

Closed mreineck closed 1 year ago

mreineck commented 1 year ago

This is my idea how the interface for more general arrays and data types could look like. Comments are very welcome!

mreineck commented 1 year ago

Here is the updated interface version I'm currently playing with. @ziotom78, does this look OK to you?

For extensibility in the future it might be nice to have submodules, like "Ducc0.fft","Ducc0.sht" etc. Would this be doable?

ziotom78 commented 1 year ago

For extensibility in the future it might be nice to have submodules, like "Ducc0.fft","Ducc0.sht" etc. Would this be doable?

Yes, you can have submodules for this: https://docs.julialang.org/en/v1/manual/modules/#Submodules-and-relative-paths

mreineck commented 1 year ago

This PR has been somewhat overtaken by the ongoing interface work by Leo and myself in the ducc main repo. But we plan to move the high-level interface part to this repo soon. I'll apply your suggestions to the new code and will most likely open another PR afterwards!

mreineck commented 1 year ago

Sorry, I forgot... could you please invite @LeeoBianchi to the repo?

ziotom78 commented 1 year ago

Sorry, I forgot... could you please invite @LeeoBianchi to the repo?

Done, I sent him an invitation!

mreineck commented 1 year ago

One more thing: if I understand correctly what I've read so far, it is custom in Julia that modules "export" the functions that are of interest to users. Doing a using Ducc0 will then imject these names into the global namespace. So we have to be somewhat careful not to use too generic names, correct?

I was actually considering renaming, e.g., nufft_nu2u to nu2u, but if the function ends up in the global namespace, that's probably not such a good idea ...

ziotom78 commented 1 year ago

One more thing: if I understand correctly what I've read so far, it is custom in Julia that modules "export" the functions that are of interest to users. Doing a using Ducc0 will then imject these names into the global namespace. So we have to be somewhat careful not to use too generic names, correct?

There are three ways to import stuff in Julia:

using Ducc0        # This brings everything in the global namespace
using Ducc0: nu2u  # This only imports what you need, but still in the global namespace
import Ducc0       # This requires users to prepend Ducc0 to everything, e.g., `Ducc0.nu2u`

I would say that it's good to have short names like nu2u; if one fears of name clashes, they should simply write import Ducc0 instead of using Ducc0.

A good example of this practice is Unitful.jl; since it defines several measurement units with just one character (m for meters, s for seconds…), typing using Unitful will not bring them to the global namespace. You have to explicitly import them:

# We're going to just use `m` and `s`, in the script, and we avoid
# other stuff like `J` (Joule), `W` (Watt), `rad` (radians), etc.
using Unitful: m, s

x = 10m
t = 5s
speed = x / t

You can see that they did not define units using symbols like unitful_m, unitful_s, because the code would have been not as readable as nice as this. (Julia favors mathematical readability when possible.)

mreineck commented 1 year ago

Thanks! OK, then I'll go ahead with the shorter names.

mreineck commented 1 year ago

I think we are now at the point where I have to update ducc0_jll.jl before we can continue. I'll try to get that done tomorrow!

mreineck commented 1 year ago

Seems like Github Actions is broken or extremely busy at the moment. I expect it will take some time until the new Yggdrasil packages are available.

ziotom78 commented 1 year ago

Seems like Github Actions is broken or extremely busy at the moment. I expect it will take some time until the new Yggdrasil packages are available.

Yes, I did several commits on a couple of repositories this morning, and all of them failed because of timeouts on Azure servers serving Ubuntu packages.

mreineck commented 1 year ago

OK, Yggdrasil seems up and running again. I switched to the current release there (called 0.27.9 so that we have some room for corrections before 0.28, which should become the "real" release). Things seem to work, but I'm not sure at all if I did the right thing in Project.toml.

mreineck commented 1 year ago

Clarification: the "Sht" testset does not work yet, because it requires an additional tweak that I made after the Yggdrasil tag.

mreineck commented 1 year ago

I think we are getting close to ready ...

The new file AbstactInterfaces.jl is my first attempt at #3 . I'm reluctant to put this code into Ducc0.jl itself, as it comes with additional dependencies, but I'm not sure how to present it properly to the outsied world.

Unless there are still open issues regarding ducc0_jll, I plan to release ducc0 and ducc0_jll v0.28 in the next few days. Then we have the freedom to proceed with this packag as well.

mreineck commented 1 year ago

How can we proceed with this? @LeeoBianchi volunteered to write docstrings for the SHT part, but that can maybe done in a separate PR. The remaining docstrings should be in place now.

I'm not sure I can provide a comprehensive test suite. Given that the algorithms themselves are continuously tested via the Python interface, I don't know how many and which sort of tests we need for Julia.

ziotom78 commented 1 year ago

I'm not sure I can provide a comprehensive test suite. Given that the algorithms themselves are continuously tested via the Python interface, I don't know how many and which sort of tests we need for Julia.

I agree, if the math is already checked in the original repository, I believe that just the most basic tests that check that functions can be properly called from Julia (e.g., there are no missing parameters in ccall) should be enough.

mreineck commented 1 year ago

OK, ducc0_jll 0.29 is out and contains the last interface changes I wanted to make.

I suggest that we merge this now and go public. There are still a lot of things to be done for sure, but I think we are at a point where additional eyes can be useful.