timholy / ANTsRegistration.jl

Convenience wrapper for image registration using the Advanced Normalization Tools
Other
11 stars 8 forks source link

Revamp Package #18

Open Dale-Black opened 3 months ago

Dale-Black commented 3 months ago

Thank you for all of your work in Julia as whole! This package seems like it would be very useful for some of the stuff I am currently doing. Do you have an idea of how hard it would be to revamp this package and get it working for modern Julia and registered? If you wouldn't mind laying out a list of things needed for that to happen, I would happy to chip away at them with various PRs if that's something you'd be interested in!

mdhe1248 commented 1 month ago

I also use ANTs time to time. I made a fork from this repository and am trying a couple of things.

timholy commented 1 month ago

Thanks @mdhe1248! I'm often flaky about checking my GitHub notifications (more science-focused these days 🙂, though that hasn't extended to this package). In case I don't respond, I've sent you an invitation so that you should be able to commit to this package.

Do you have an idea of how hard it would be to revamp this package and get it working for modern Julia

I haven't used it in ages, and no one has reported that it doesn't work for modern Julia. Since I don't even know what the problems are, no clue 😆.

get it registered

I'm open to that. It wasn't even registered in HolyLabRegistry.jl, but if there's demand it could either go there or in General.

mdhe1248 commented 1 month ago

@timholy If I remember correctly, the core of this package worked well. I just wanted to include more options and functionalities to it (e.g. applying inverse transformation). Well, the options I'd like to include are the ones that I find useful for my research haha :). I have been learning a lot about programming styling by reading your source code. One issue I am unsure how to resolve is setting up a newer version of ANTs binary. Another thing to consider is the coding style for adding extra options and functionalities (e.g. should the input argument be structured as Struct or Tuple?). I will update some of the code in the next couple of weeks and submit a merge request.

P.S. link to ANTs binaries: https://github.com/ANTsX/ANTs/releases

timholy commented 1 month ago

deps/build.jl is what handles the binary. But long-term, the most robust solution might be to get it into https://github.com/JuliaPackaging/Yggdrasil. That would result in an ANTS_jll.jl package that would allow direct access to the C API. Though it looks like ANTs is mostly written in C++, which is much more challenging to wrap. That would probably be a substantial project, so perhaps updaing the build script is the easier way to go.

Generally I'd say make additional options into kwargs. But if there is a whole set of options that are handled as a unit, I'd make that a struct. (You can do validation checking on a struct in a way that's hard to do with a NamedTuple.)

mdhe1248 commented 1 month ago

A couple of days ago, I attempted to modify deps/build.jl, but it did not work. As a temporary solution, I removed build.jl and separately installed ANTs on my laptop - which works well with minor changes in your original source code.

To make things clean, I will need to spend a bit more time on the binary file. I thought it might be related to the extension of the binary file. In the ANTs website, the file extension is .zip. In the current build.jl, the download source is from https://github.com/IanButterworth/bins/blob/master/3rdparty/ANTS/2.1.0/ and all binary files are tar.gz

I will first add a couple of more options/functions and then work on the binary file :) (btw, i did not know about NamedTuple until now. Would you recommend struct over NamedTuple?).

timholy commented 1 month ago

Yeah, if the filenames have changed then the script does need updating.

NamedTuple is basically an infinitely-flexible struct. Use it when you need that flexibility. Its most important role is in handling kwargs:

julia> foo(; kwargs...) = kwargs
foo (generic function with 1 method)

julia> foo(; a=5, b="hello")
pairs(::NamedTuple) with 2 entries:
  :a => 5
  :b => "hello"

But if the fields are known in advance, and especially if they have some constraints (e.g., a field n cannot be negative, or allowed settings for field mode depend on field data), you typically want struct.