pyxem / pyxem

An open-source Python library for multi-dimensional diffraction microscopy.
https://pyxem.readthedocs.io
GNU General Public License v3.0
147 stars 85 forks source link

Removing (some) Generators From PyXEM #776

Open CSSFrancis opened 3 years ago

CSSFrancis commented 3 years ago

Removing Generators From PyXEM

I think that the class of generators in PyXEM is more confusing that it is helpful. From a pure semantics point of view in python a generator is a function which yields a result rather than returns a result.

In our case generators are basically self-contained workflows which could be better expressed by example notebooks rather than by constrained notebooks.

For something like the Calibration Generator:

get_elliptical_distortion --> added to the Diffraction2D class get_distortion_residuals --> added to Diffraction 2D plot_corrected_diffraction_pattern --> described in workflow notebook get_diffraction_calibration --> described in workflow notebook

This takes some of the pressure on our end to support large blocks of the code base which are often sticking points.

For the Other Generators:

displacement_gradient --> Show this as work flow. Maybe function of diffraction_vectors index_generator --> This might be the only one that I might keep. But maybe this makes more sense being passed to a electron diffraction function integration_generator --> Moved to function of diffraction_vectors pdfGenerator --> function of ReducedIntensity1D red_intensity_generator --> function of ElectronDiffraction1D SubpixelrefinementGenerator --> Function of ElectronDiffraction2D VarianceGenerator --> Already Duplicated VirtualImageGenerator --> Show as a work flow

For the most part I think that we should depreciate each of the workflows above and then move towards something more cohesive and sustainable where signals have functions rather than creating separate objects that take signals as arguments unless they need to.

My proposal is that we shoot for a 1.0 release of this where we have some major changes with the stable release.

I might start working on this in the meantime unless there is any major augments against this.

pc494 commented 3 years ago

I think some generators (generally those that take two complicated pyxem classes) are an asset to the code. That said I think we've been a bit generous with what we make into a generator and this suggestion will certainly improve the code.

Eg:

get_elliptical_distortion --> added to the Diffraction2D class as a method get_distortion_residuals --> added to Diffraction 2D as a method

are obviously improvements.

Similarly I think objects that do a single thing (eg displacement_gradient_generator) can be turned into functions with little downside.

I'm a little on the fence about cases where several similar methods would exist (clogging up the namespace) - for example SubpixelrefinementGenerator.

This comment should mainly considered a guide as to what I think are the safest changes (ie. where to maybe start)

CSSFrancis commented 3 years ago

@pc494 thanks for the input. I'll see if I can start working on this once I get some of my current PRs merged. I still have some cleaning up there before I start something new.

pc494 commented 3 years ago

You're doing a good portion of the development at the moment, so I'm not at all worried about timings.

magnunor commented 2 years ago

I think some generators (generally those that take two complicated pyxem classes) are an asset to the code.

After having worked a bit with the generators a bit, I agree with this design philosophy. It addresses an old problem in pixstem, when working with diffraction vector data, which needed access to both the vector and the diffraction data at the same time.

After the current release is sorted out (0.14), and the DiffractionVector class is improved (via VectorSignal https://github.com/hyperspy/hyperspy/pull/2876), we should go through the generators and see what works best.