Adding ND rebinner to refactor_24

[pmneves7]

This pull request adds a new function called NDrebin to the transforms folder. The goal of NDrebin is to enable the rebinning of an arbitrary sampling of datapoints in N-dimensions to a regular grid in N-dimensions. This is highly useful for arbitrary data visualization when the data was not measured on a nice uniform set of points, as is the case for example with (qx,qy,qz) points measured on a SANS instrument.

Note that this function will somewhat average or interpolate data, so some resolution information is lost. The ideal case is to fit or model directly to measured datapoints, then use NDrebin on both the data and the fit to produce plots and visualizations that can be compared.

Also added a utest_NDrebin file to test the NDrebin function. Additional manual tests can be performed by running the NDrebin.py file as main.

[krzywon]

Ideally, NDrebin would be a class instead of a function, and many of the smaller calculation elements would be their own class methods. The many checks for dimensionality, etc, could all be done in the __init__. A series of smaller class methods, each specific to a single calculation, would greatly improved maintainability, and likely clear up the CodeScene complaints.

class NDRebin:

    def __init__(data: Quantity[ArrayLike],
            coords: Quantity[ArrayLike],
            data_errs: Quantity[ArrayLike] | None = None,
            axes: list[Quantity[ArrayLike]] | None = None,
            upper: list[Sequence[float]] | None = None,
            lower: list[Sequence[float]] | None = None,
            step_size: list[Sequence[float]] | None = None,
            num_bins: list[Sequence[int]] | None = None,
            fractional: bool | None = False
    ):
        self.data = data
        ...
        self.binned_data = np.zeroes(self.data.shape())  # I'm not sure this is actually correct.

    def __call__():
        self.calculate_bins()

    def calculate_bins():
        # This would be the main call

    def _calculate_fractional_binning():
        # Put the fractional binning calcs in here.
        

[pmneves7]

Still refactoring the documentation and tests.

The new class-based syntax is:

rebin = NDRebin(I_ND, qmat,
    step_size=0.2*np.random.rand(Ndims)+0.1,
    lower=[1,2,3],
    upper=[9,8,7])
rebin.run()
Ibin = rebin.binned_data
qbin = rebin.bin_centers_list

I am wondering if the rebin.run() should just be automatically done when the class is constructed. To me this makes sense but maybe there are object oriented reasons not to?

[pmneves7]

Should be ready to be reviewed again @krzywon

[DrPaulSharp]

With respect to rebin.run(), the advantage of separating the run code from initialisation is that it enables the rebinner to be reused after it has been initialised. With further refactoring (which I am not suggesting for this PR) it would be possible to rebin with different parameters, for example:

rebin.run(nbins=30)
rebin.run(nbins=40)

Given this, I'd suggest leaving the code as is regarding running the rebinner.

I've got a few comments, I'll let @krzywon do the full review.

[krzywon]

Overall, much cleaner code. I haven't tested functionality, but unit tests pass. I think the comments and questions posed by @DrPaulSharp are valid and should be addressed

[pmneves7]

With respect to rebin.run(), the advantage of separating the run code from initialisation is that it enables the rebinner to be reused after it has been initialised. With further refactoring (which I am not suggesting for this PR) it would be possible to rebin with different parameters, for example:

rebin.run(nbins=30)
rebin.run(nbins=40)

Given this, I'd suggest leaving the code as is regarding running the rebinner.

I've got a few comments, I'll let @krzywon do the full review.

This would still be the case if rebin.run() is called in __init__ right? You could call run again later if you want to change the limits etc. Adding run to the initialization just saves one line when you define the rebin object the first time. In other words, when would someone want to create a rebin object and not want to then run it? Maybe I am overthinking this though, and if you think it is problematic we can keep it as is.

[DrPaulSharp]

With respect to rebin.run(), the advantage of separating the run code from initialisation is that it enables the rebinner to be reused after it has been initialised. With further refactoring (which I am not suggesting for this PR) it would be possible to rebin with different parameters, for example:

rebin.run(nbins=30)
rebin.run(nbins=40)

Given this, I'd suggest leaving the code as is regarding running the rebinner.
I've got a few comments, I'll let @krzywon do the full review.

This would still be the case if rebin.run() is called in __init__ right? You could call run again later if you want to change the limits etc. Adding run to the initialization just saves one line when you define the rebin object the first time. In other words, when would someone want to create a rebin object and not want to then run it? Maybe I am overthinking this though, and if you think it is problematic we can keep it as is.

Hmm, given that you have written a __call__ method, I think keeping the separation of functionality from initialising and running is the best approach here. Fundamentally the approach with the code as it stands allows for greater flexibility which is where the advantage is.

[pmneves7]

With respect to rebin.run(), the advantage of separating the run code from initialisation is that it enables the rebinner to be reused after it has been initialised. With further refactoring (which I am not suggesting for this PR) it would be possible to rebin with different parameters, for example:

rebin.run(nbins=30)
rebin.run(nbins=40)

Given this, I'd suggest leaving the code as is regarding running the rebinner.
I've got a few comments, I'll let @krzywon do the full review.

This would still be the case if rebin.run() is called in __init__ right? You could call run again later if you want to change the limits etc. Adding run to the initialization just saves one line when you define the rebin object the first time. In other words, when would someone want to create a rebin object and not want to then run it? Maybe I am overthinking this though, and if you think it is problematic we can keep it as is.

Hmm, given that you have written a __call__ method, I think keeping the separation of functionality from initialising and running is the best approach here. Fundamentally the approach with the code as it stands allows for greater flexibility which is where the advantage is.

Very well, then we can keep it as is.

[pmneves7]

@DrPaulSharp @krzywon Are we ready to merge this branch in? I think I addressed all of your concerns.

[DrPaulSharp]

@DrPaulSharp @krzywon Are we ready to merge this branch in? I think I addressed all of your concerns.

I'll take a proper look soon, but I need you to do a job for me first. Could you please rebase this branch on top of the refactor branch using git rebase refactor_24 with the refactor_24_NDrebin branch checked out. I've been sorting the branches recently so this one has become out of date.

[pmneves7]

@DrPaulSharp @krzywon Are we ready to merge this branch in? I think I addressed all of your concerns.

I'll take a proper look soon, but I need you to do a job for me first. Could you please rebase this branch on top of the refactor branch using git rebase refactor_24 with the refactor_24_NDrebin branch checked out. I've been sorting the branches recently so this one has become out of date.

I think I have rebased the branch, though I am not a git ninja.

[DrPaulSharp]

I think I have rebased the branch, though I am not a git ninja.

That seems to have got it although github has not tidied up the PR unfortunately. The PR currently has +794 lines -0 lines, with two files modified. Is this what you expect for the work you have done here?

Hmm, there are 592 commits, which I don't like the look of.

[codescene-delta-analysis]

No quality gates enabled for this code.

See analysis details in CodeScene

Quality Gate Profile: Custom Configuration
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[DrPaulSharp]

Ok, that sorts the rebase. I'll give the code a once over later today.

[DrPaulSharp]

@krzywon may want another look, but all seems ok to me.