Add a Spectrum type #70

acolley · 2017-07-21T19:16:53Z

This is a PR for issue #64.

I had a need for a Spectrum class for a Ray Tracer project I'm working on and I noticed the existing PR had gone stale about a year ago. I didn't know if there was a way to edit that existing PR so I made this one.

I've based most of it off of the PR by tatref: #65. In addition I added the functions mentioned in the comments on that PR including the generic function for the mapping LUT and what I assumed was the 'gradient' method, which creates a sampling of a given input mapping wavelength to intensity using linear interpolation.

I've also added error handling using the error-chain library.

This adds two new dependencies to the library: ordered-float and error-chain.

One thing I wasn't sure of: what should the name of the 'gradient' method be? Currently it's called 'from_sparse' but I don't think that really reflects what it's doing.

The testing is also not perfect and I couldn't think of a great way of testing this sort of thing except some simple cases.

Ogeon

Thanks for picking this up! I'm not sure why I stopped responding to the other PR, but it seem to coincide with me moving to a new apartment and having a lot to do at work. (Sorry @tatref) It was a period where I had too much to hold in my head at the same time. 😞

Either way, I think this can be a good addition and that it should be as integrated as possible. That means that I think it should be considered a color format and be able to be converted to other color formats, in addition to xyz. (See inline comment)

I would also like to bring up the semantics of adding, subtracting, dividing and multiplying spectra with each other. What does one expect from one_red / another_red + some_green? The same as in RGB? If that's the case and we implement those operators for RGB, then go for it!

Oh, and an operation that we could add is to be able to add (and maybe subtract, multiply and divide) spikes/pulses. Like a single wavelength. Either that or some other smart way to do single sample operations, just like you can manipulate the components of the other color formats.

As for the name of from_sparse, I'm not a fan either. Maybe something like from_samples or from_interpolated? Naming is not my main strength...

Ogeon · 2017-07-22T17:00:12Z

src/spectrum.rs

+pub const SPECTRUM_MAX_LAMBDA: f32 = 830.0;
+
+pub const SPECTRUM_SAMPLES: usize = 95;
+const SPECTRUM_SAMPLE_STEP: usize = 5;


Where are these numbers derived from? Can if they are related to each other, can they be expressed as a constant expression?

I've refactored the constants to use constant expressions and removed the redundant 'SPECTRUM' naming (as this is obvious given we're in the 'spectrum.rs' module).

Ogeon · 2017-07-22T17:08:35Z

src/spectrum.rs

+        let mut data: [T; SPECTRUM_SAMPLES] = [T::zero(); SPECTRUM_SAMPLES];
+        for (sample, cloned) in self.data.iter().zip(data.iter_mut()) {
+            *cloned = *sample;
+        }


I guess it's practically the same, but perhaps clone_from_slice could be used here. It could at least help readability a bit.

Cool! Didn't know about that method, though it seems there's also copy_from_slice which it suggests might be faster so I've used that.

Ogeon · 2017-07-22T17:18:27Z

src/spectrum.rs

+    /// values assumed to be in the range 360 nm to 830 nm.
+    pub fn new(data: [T; SPECTRUM_SAMPLES]) -> Result<Self> {
+        if data.iter().any(|&intensity| intensity < T::zero()) {
+            Err(ErrorKind::SpectrumIntensityOutOfRange.into())


Did you consider using an assert! or a debug assertion here instead? The question is who is responsible for checking the input? The library of the user? I don't remember if there are any other occurrences of this, but if we decide to keep it like this, then we could perhaps add _uchecked versions of the constructors.

I guess if we were going with what the rest of the library does then we should use assert! and expect the caller to make sure there are no invalid values in the array.

I've updated the code to do this and that has removed the need for error-chain and the errors.rs module which I've also removed.

Ogeon · 2017-07-22T17:21:02Z

src/spectrum.rs

+    /// The more data points there are the more accurate the 
+    /// `Spectrum`'s internal representation will be.
+    pub fn from_sparse(data: &[(f32, T)]) -> Result<Self> {
+        // TODO: replace this with sort_unstable_by() when stabilised.


I would rather have this todo in an issue when this is done. It won't be fixed anytime soon if it's hidden in the code.

Ogeon · 2017-07-22T17:35:00Z

src/spectrum.rs

+    /// 
+    /// The more data points there are the more accurate the 
+    /// `Spectrum`'s internal representation will be.
+    pub fn from_sparse(data: &[(f32, T)]) -> Result<Self> {


This looks a lot like what Gradient is doing. Could it be used internally and/or be taken as input somehow?

Also, is the step size small enough for supersampling to be unnecessary? I seem to remember that I used the area of the wavelength bin as a weight in my own path tracer toy, but I sampled multiple wavelengths from the start, instead of RGB. May be worth investigating, though, since it may give a better representation of spikes.

Ogeon · 2017-07-22T17:39:57Z

src/spectrum.rs

+    }
+
+    /// Converts a `Spectrum` to `Xyz` tristimulis values.
+    pub fn to_xyz<Wp: WhitePoint<T>>(&self) -> Xyz<Wp, T> {


I have been away for some time, so correct me if I'm not remembering everything, but there is a generic color conversion trait in here, somewhere. I think Spectrum could implement that. It should have a bunch of semi automatic implementations to other formats, as well, so it should become fully compatible with other parts of the library. You may have to add some macro invocations somewhere for that to work 100%.

…edundancy in the naming

…more refactoring

Ogeon

Sorry for my delayed response. I'm visiting family, so my online presence is a bit spotty. Anyway, the changes looks good 👍, but there are some missing documentation and some naming that I think can be improved.

Ogeon · 2017-07-29T12:42:24Z

src/spectrum.rs

+/// The smallest wavelength represented in the `Spectrum`
+/// data structure. This is the first value in the
+/// map's wavelength.
+pub const MIN_LAMBDA: usize = 360;


I was thinking that perhaps we should name these {MIN, MAX}_WAVELENGTH, since that's what they are, instead of which letter is used to represent them.

I had the exact same thought. Will push this change in the next commit, though I was thinking we might also want to include the units? (e.g. MAX_WAVELENGTH_NM).

Ogeon · 2017-07-29T12:47:41Z

src/spectrum.rs

+
+const SAMPLE_STEP: usize = 5;
+
+pub const N_SAMPLES: usize = (MAX_LAMBDA - MIN_LAMBDA) / SAMPLE_STEP + 1;


It's something with the N_ prefix... Perhaps just SAMPLES is good enough? Or is that ambiguous? SECTRUM_LENGTH? SECTRUM_LEN, to follow the .len() pattern?

Ogeon · 2017-07-29T12:53:25Z

src/spectrum.rs

+    /// The intensity values must be greater than or equal to
+    /// zero and not NaN.
+    ///
+    pub fn from_samples(data: &[Sample<T>]) -> Spectrum<T> {


This could take &[S] where S: Into<Sample<T>>. And Sample<T> could implement From<(f32, T)>.

Sounds good. I'm still learning about all the power Rust gives you for creating convenient APIs.

acolley · 2017-07-30T14:36:23Z

Another design question I had was about whether we should allow the consumer of the API the ability to choose how many samples are stored in a Spectrum. Currently this is hard coded as 95 samples, but comparing this to pbrt for example. That only uses 30 samples, presumably because this is a happy medium between accuracy and speed but when compiling that you can just set that number to be as high or low as necessary.

Obviously that wouldn't work for a library like palette. However I can't think of any way of allowing that sort of control at creation time short of using a Vec to store a dynamic number of samples. Though that would lose the flexibility of a consumer being able to choose if a Spectrum should be stack or heap allocated.

It would also affect the performance of the various arithmetic operators as you would have to convert the two Spectrums into the same sample space to do the operation each time.

Perhaps we should consider dropping the number of samples from 95 to some lower number for the initial release of this feature to avoid the overhead of extra accuracy that the majority of users might not need?

In addition, exposing a constructor (i.e. Spectrum::new) that can accept a fixed-size array we are exposing the internals and wouldn't be able to adjust the samples in the future without breaking API compatibility. Perhaps we should just have the from_samples method that would enable any size of input data to be accepted without exposing how it's stored internally.

Sorry that was a bit of a mind dump :)

Ogeon · 2017-07-30T14:55:38Z

I think it's fine to skip the unit. It's common to use nanometers and it can be clarified in the documentation.

As for the resolution of the spectrum, I think it's a nice idea, but the problems with combining spectra makes it less worth it. It could work if/when Rust gets type level integers. Until then we could use empty types to define different sizes (see how color spaces works), but that's not as flexible.

Do you have a working project that uses this? Would you like to do a bit of research and see if >30 makes a lot of difference? If it is a happy medium, then perhaps that's a good starting point. One thing I'm not sure about is how it works with conversion to and from XYZ. I guess each resolution would need its own curve data.

acolley · 2017-08-01T09:38:25Z

Just an update to say I'm still working on this. Currently working on the finishing touches to a supersampling method for sampling the input data as mentioned in a previous review comment.

Ogeon · 2017-08-08T09:22:11Z

Nice! There are still a couple of undocumented items, but it looks like I haven't added any automatic checks for that yet. Something that would be a nice addition is a small example in the examples directory. Maybe just a simple rainbow, or something. Otherwise I think it's a good first version of a spectrum type.

Ogeon · 2017-08-08T18:06:14Z

I have added a new merge bot that replaces Homu, so I think it's best if you rebase your branch to the latest master at some point (just to be on the safe side). Make sure bors.toml is in your branch, and it should be good.

Ogeon · 2018-02-17T14:51:12Z

@acolley I guess this one was forgotten. Are you still interested?

Ogeon · 2018-04-20T18:07:56Z

I'll close this due to inactivity, but feel free to pick it up again and continue if you ever want to.

acolley added 3 commits July 20, 2017 20:18

Add implementation of Spectrum type

05089f6

Add error handling using error_chain

df2b837

add missing errors.rs

b56d514

Ogeon requested changes Jul 22, 2017

View reviewed changes

acolley added 6 commits July 23, 2017 11:30

Use copy_from_slice instead of manual copy

4259c64

Refactor sample constants to use compile time expression and remove r…

9f1bd57

…edundancy in the naming

Change error Result to be a panic on assert instead

087ff69

Adding some docs to Spectrum methods

2bcbfbe

Rename from_sparse to from_samples

fb1ed31

Add Sample type and refactor Spectrum to use this in preparation for …

cb84529

…more refactoring

Ogeon requested changes Jul 29, 2017

View reviewed changes

acolley added 4 commits August 1, 2017 22:19

Implement weighted average of samples in from_samples

38d41ab

Rename N_SAMPLES to SAMPLE_COUNT

f2055c3

Remove unnecessary Debug trait bound

56f7fff

Add from impl for converting tuple into a Sample

7c5f3c9

Ogeon approved these changes Aug 8, 2017

View reviewed changes

Ogeon mentioned this pull request Aug 8, 2017

WIP initial implementation of spectrum to xyz conversion #65

Closed

Ogeon added this to the 0.3.0 milestone Aug 8, 2017

Ogeon removed this from the 0.3.0 milestone Feb 17, 2018

Ogeon closed this Apr 20, 2018

Ogeon mentioned this pull request Feb 17, 2024

Derrington-Krauskopf-Lennie (DKL) color space support #376

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Spectrum type #70

Add a Spectrum type #70

acolley commented Jul 21, 2017

Ogeon left a comment •

edited

Loading

Ogeon Jul 22, 2017

acolley Jul 23, 2017

Ogeon Jul 22, 2017

acolley Jul 23, 2017

Ogeon Jul 22, 2017

acolley Jul 23, 2017

Ogeon Jul 22, 2017

Ogeon Jul 22, 2017

Ogeon Jul 22, 2017

Ogeon left a comment

Ogeon Jul 29, 2017

acolley Jul 30, 2017

Ogeon Jul 29, 2017

Ogeon Jul 29, 2017

acolley Jul 30, 2017

acolley commented Jul 30, 2017

Ogeon commented Jul 30, 2017

acolley commented Aug 1, 2017

Ogeon commented Aug 8, 2017

Ogeon commented Aug 8, 2017

Ogeon commented Feb 17, 2018

Ogeon commented Apr 20, 2018


		const SAMPLE_STEP: usize = 5;

		pub const N_SAMPLES: usize = (MAX_LAMBDA - MIN_LAMBDA) / SAMPLE_STEP + 1;

Add a Spectrum type #70

Add a Spectrum type #70

Conversation

acolley commented Jul 21, 2017

Ogeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolley commented Jul 30, 2017

Ogeon commented Jul 30, 2017

acolley commented Aug 1, 2017

Ogeon commented Aug 8, 2017

Ogeon commented Aug 8, 2017

Ogeon commented Feb 17, 2018

Ogeon commented Apr 20, 2018

Ogeon left a comment •

edited

Loading