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

Improve handling of spectral units #311

Merged
merged 12 commits into from
Jul 30, 2024
Merged

Improve handling of spectral units #311

merged 12 commits into from
Jul 30, 2024

Commits on Jul 25, 2024

  1. Create "reciprocal_spectroscopy" units context

    - Pint tends to choke on conversions between reciprocal energy and
      wavenumber. These don't come up too often but are an issue when
      dealing with histograms and spectra.
    
    - Here we add a few more "definitions" for Pint to use in such cases.
    
    - This creates a slightly scary range of paths around unit
      conversions. By putting them in a separate context, we can ensure
      they are only introduced when they are likely to be needed.
    ajjackson committed Jul 25, 2024
    Configuration menu
    Copy the full SHA
    d3b526f View commit details
    Browse the repository at this point in the history

Commits on Jul 26, 2024

  1. Use proper units for imported CASTEP DOS

    DOS units should be reciprocal of energy/frequency axis: this gives
    dimensionless area (count) and scales properly with unit changes.
    
    It looks like DOS was expressed in enegy/frequency units in part to
    avoid some Pint conversion challenges. The new reciprocal_spectroscopy
    context should allow those to be done more safely and directly.
    
    There are still some more test failures to be examined and cleaned
    up.
    ajjackson committed Jul 26, 2024
    Configuration menu
    Copy the full SHA
    871ae4d View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    0f6e69c View commit details
    Browse the repository at this point in the history

Commits on Jul 29, 2024

  1. Rework broadening internal units handling

    - Broadening logic tries to convert to hartree and back again, but
      this behaves unpredictably (or otherwide badly) for non-energy
      units.
    
    - Instead we can use the bin units; this still requires that the
      relevant parameters are dimensionally-consistent _with each other_.
    
    - While working on this I noticed that very small bin
      values (i.e. when using large bin _units_) are incorrectly
      identified as having equal width due to the "atol" which is intended
      to stabilise comparisons near zero. Bin widths should always be
      finite, so we can make this stricter and always base agreement on
      the relative width.
    ajjackson committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    dfa71d8 View commit details
    Browse the repository at this point in the history
  2. CHANGELOG update

    ajjackson committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    029e617 View commit details
    Browse the repository at this point in the history
  3. Faster, more robust spectrum getters

    The current spectrum getters create a unit conversion factor and then
    multiply the raw array by it. There are two problems with this:
    
    - Multiplying any iterator by a Quantity or Unit leads to Pint
      checking through all the data for consistency. This can be quite
      expensive, and is unnecessary for these trusted array objects. When
      iterating over spectra yielded from a Spectrum1DCollection, the cost
      can become quite noticeable.
    
    - Some unit conversions in the spectroscopy context involve taking
      a reciprocal, e.g. conversion from cm -> 1/cm is allowed. But if
      this is done to a _factor_ the data itself will not be inverted. If
      we start with a Spectrum1D with x_data in wavenumber (1/cm) then
    
        spectrum.x_data.to("cm")
    
      and
    
        spectrum.x_data_unit = "cm"
        spectrum.x_data
    
      will not give the same result; the second method leaves the actual
      values untouched (as cm / (1/cm) = 1).
    ajjackson committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    ac02092 View commit details
    Browse the repository at this point in the history

Commits on Jul 30, 2024

  1. Add full set of length^-1 <-> energy conversions to rs context

    By providing Pint with "direct" conversion routes between wavenumber
    and energy (in each direction and inversion), Pint avoids
    unnecessarily taking the reciprocal of data and encountering
    divide-by-zero/infinite-value situations.
    
    It doesn't look like the existing divide-by-zero situations were
    causing problems with correctness of results, but its nice to clear
    the warnings (and save some CPU?) so that when we see this warning we
    know it is worth investigating.
    ajjackson committed Jul 30, 2024
    Configuration menu
    Copy the full SHA
    f39424d View commit details
    Browse the repository at this point in the history
  2. Use parens to force reciprocal_spectroscopy application order

    Performance tests show this gives a significant improvement for calls
    to `.to()` and slightly worse performance for (the much-faster) `.ito()`.
    ajjackson committed Jul 30, 2024
    Configuration menu
    Copy the full SHA
    960ae06 View commit details
    Browse the repository at this point in the history
  3. Cleanup for linter

    ajjackson committed Jul 30, 2024
    Configuration menu
    Copy the full SHA
    78961e0 View commit details
    Browse the repository at this point in the history
  4. CHANGELOG update

    ajjackson committed Jul 30, 2024
    Configuration menu
    Copy the full SHA
    fa0e382 View commit details
    Browse the repository at this point in the history
  5. Use a non-returning function for unit conversion check

    Reviewer found the ``_ =`` distracting, but it is nice to clarify here
    that we don't care about the return value. Switching to ``.ito()``
    also achieves that (and comes with a miniscule performance advantage.)
    ajjackson committed Jul 30, 2024
    Configuration menu
    Copy the full SHA
    890ac91 View commit details
    Browse the repository at this point in the history
  6. Use toolz for a cleaner data transformation in test suite

    I hope to use toolz more in future, for now it is just needed for this
    one test!
    ajjackson committed Jul 30, 2024
    Configuration menu
    Copy the full SHA
    517e8d0 View commit details
    Browse the repository at this point in the history