Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-uniform axes progress tracking #2398

Open
16 of 39 tasks
francisco-dlp opened this issue May 19, 2020 · 13 comments
Open
16 of 39 tasks

Non-uniform axes progress tracking #2398

francisco-dlp opened this issue May 19, 2020 · 13 comments

Comments

@francisco-dlp
Copy link
Member

francisco-dlp commented May 19, 2020

This issue tracks progress on the implementation of non-uniform axes.

Notice that, currently, the non-uniform axes features are merged into the non_uniform_axes branch.

If you intend to address any of the issues below, please discuss it here to avoid overlapping of efforts.

Key issues to address before release

Issues that would be nice fixing before releasing, but should not hold a release:

Remaining issues

  • Enable non-uniform axes support in usid_hdf5
  • shift1D
  • align1D (estimate_shift1D)
  • shift2D
  • align2D
  • EELS:
    • Low-loss convolution
    • Fourier-log deconvolution
    • Fourier-log deconvolution
    • Richardson-lucy deconvolution
    • EELSCLEdge fine structure
  • Holography features
@jlaehne
Copy link
Contributor

jlaehne commented May 19, 2020

  • +1 for using pcolormesh for image plotting. I am already using that in my own scripts to plot e.g. spectral linescans due to its support for non uniform data spacing.
  • +1 for deprecating metadata.Signal.binned and introducing is_binned as attribute of the axis.

(I wanted to add rebin as low hanging fruit, but you already did so)

@francisco-dlp
Copy link
Member Author

Notice that the list above is certainly non-exhaustive. Please add any remaining issues that I may have missed.

@jlaehne
Copy link
Contributor

jlaehne commented May 19, 2020

From #2299 (comment):

Other optional issues that might be included concern the improvements to the axes_manager suggested by @thomasaarholt in #2345 (trying to summarize them in a checklist):

  • Introduce possibility to initialize both Signal and AxesManager by passing an axis object in place of a dictionary (would make it easier to copy an axis from one signal to another) (done in hspy read support for non linear axis and more error catches #2304)
  • Protect size of axes in some form so that it can not be changed unless the dimensions of the data are also changed (do we need to do this fool-proofing)?.
  • Make data axis iterable.
  • Further improve axes_manager documentation (based on points raised in Ambiguity on the Signal axes argument #2345).

@jlaehne
Copy link
Contributor

jlaehne commented Jun 9, 2020

When plotting data with a non-uniform signal axis, the navigator still uses sum() to create the spectrum/image for navigation, which now gives a warning. Would make sense to replace sum() by integrate1D (which however takes an axis number instead of an axis object as input). [does not hold a release, but would be nice to fix].

@pquinn-dls
Copy link
Contributor

A solution we use here is for non-linear axes in ND is to Histogram everything. Every plot is just a binned representation - histogram also seems to be the fastest method (sorry if you're already doing this, haven't looked at this branch in depth)

@jlaehne
Copy link
Contributor

jlaehne commented Aug 25, 2020

_create_axis should become a public instead of a private method as proposed by @ericpre in the discussion of #2304 (done in #2304)

@thomasaarholt
Copy link
Contributor

Docstrings for all of the Axis classes should also be added.

@jlaehne
Copy link
Contributor

jlaehne commented Mar 31, 2021

Updated the checklist with all the comments, @thomasaarholt is making the data axis iterable solved by #2459?

@thomasaarholt
Copy link
Contributor

No not with that PR - it only covered the order in which one would iterate across the AxesManager. I'll try to add it soon.

@jlaehne
Copy link
Contributor

jlaehne commented May 5, 2021

I have worked on the remaining key issues and some first nice to have ones in #2728. We're finally getting close to getting this into RELEASE_next_minor.

@ericpre
Copy link
Member

ericpre commented May 31, 2021

I have updated the summary of this issue and there is possibly one point left before merging:

  • Most things that don't work on non-uniform axes are identified and an informative NotImplementedError is raised.

@jlaehne, it looks to me that you already had a good around at what is not supported. Do you think that this last point is completed?

@jlaehne
Copy link
Contributor

jlaehne commented Jun 1, 2021

@ericpre, I had 2-3 things I wanted to check/add NotImplementedErrors and a few tests for not implemented errors I wanted to add on top, but largely it should be done. I'll try to get that done this week.

There are a number of points where coverage could be increased further, but I do not see the codecov comments at the moment. Anyway, coverage is already much better than for large parts of hyperspy, so it could still be added after the merge.

@jlaehne
Copy link
Contributor

jlaehne commented Jun 3, 2021

I have updated the summary of this issue and there is possibly one point left before merging:

  • Most things that don't work on non-uniform axes are identified and an informative NotImplementedError is raised.

From my POV this last point should be resolved with #2761.

The non_uniform_axes branch has been in productive use for a number of people for a while now - so I think merging into RnM soon to increase the test user base prior to a release would be good (anyway it is soon a year since the last major release).

As mentioned, it could be nice to use the codecov comments of the overall PR to increase coverage further, but I am not sure if I would have the time to do so in the next weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants