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

Spectrum: Allow assigning None to the quantity field #336

Merged
merged 2 commits into from Jun 15, 2023

Conversation

leroyvn
Copy link
Member

@leroyvn leroyvn commented Jun 14, 2023

This PR closes #333.

Description

This commit allows leaving the Spectrum.quantity field unset. This is required to allow for incomplete Spectrum object initialization, which is, in turn, required to allow for the deferred application of a default quantity field by a converter.

The policy is as follows:

  • if quantity is set, unitless spectrum values are applied appropriate units and validators check if spectrum values have consistent units;
  • if quantity is None, unitless spectrum values are allowed and validators do not check spectrum value units, which can be anything.

Integral computations will consider unitless fields as dimensionless.

How does it work, in practice?

I refactored the Spectrum constructors to apply units, if relevant, prior to initialization. For example with UniformSpectrum:

  • if quantity is set and value is assigned a quantity, the validator makes a consistency check;
  • if quantity is set and value is assigned a unitless value, value is attached units, then the validator makes a consistency check;
  • if quantity is unset nothing happens.

If one instantiates a UniformSpectrum with unset quantity, it is possible to:

  • apply arbitrary units to the value field;
  • assign a value to the quantity field.

Both should be done with attrs.evolve() to make sure that validation occurs.

The whole point of it

The SpectrumFactory.converter() method is also updated to allow for consistent initialization of Spectrum objects in situations where a missing quantity field can be inferred from the context.

In practice, these fixes enforce symmetry between spectrum dictionary and constructor syntaxes:

import eradiate.scenes as ertsc

# 1
ground_bsdf_dict = ertsc.bsdfs.LambertianBSDF(
    reflectance = {
        "type": "interpolated",
        "wavelengths": [440, 550, 620],
        "values": [0.067269, 0.096786, 0.125058]
    }
)

# 2
ground_bsdf_obj = ertsc.bsdfs.LambertianBSDF(
    reflectance=ertsc.spectra.InterpolatedSpectrum(
        wavelengths=[440, 550, 620], 
        values=[0.067269, 0.096786, 0.125058]
    )
)

In snippet 1, the conversion protocol completes the dictionary with a "quantity" entry, then used to instantiate a well-formed InterpolatedSpectrum. In snippet 2, an incomplete InterpolatedSpectrum instance is created; the converter then evolves it into a new object for which the quantity field is specified; all unitless fields are then converted and validated.

Checklist

  • The code follows the relevant coding guidelines
  • The code generates no new warnings
  • The code is appropriately documented
  • The code is tested to prove its function
  • The feature branch is rebased on the current state of the main branch
  • I updated the change log if relevant
  • I give permission that the Eradiate project may redistribute my contributions under the terms of its license

@leroyvn leroyvn added the bug 🐛 Something isn't working label Jun 14, 2023
@leroyvn
Copy link
Member Author

leroyvn commented Jun 14, 2023

@schunkes This turned out to be harder to fix than I thought. The good thing is that it forced the definition of a policy on when to apply units to unitless fields or not.

This commit allows leaving the `Spectrum.quantity` field unset. This is
required to allow for incomplete Spectrum object initialization, which
is, in turn, required to allow for the deferred application of a
default `quantity` field by a converter.

The policy is as follows:

* if `quantity` is set, unitless spectrum values are applied appropriate
  units and validators check if spectrum values have consistent units;
  consistent;
* if `quantity` is None, unitless spectrum values are allowed and
  validators do not check spectrum value units, which can be anything.

Integral computations will consider unitless fields as dimensionless.

The SpectrumFactory.converter() method is also updated to allow for
consistent initialization of Spectrum objects in situations where a
missing quantity field can be inferred from the context.
@leroyvn leroyvn merged commit 972b3d8 into main Jun 15, 2023
@leroyvn leroyvn deleted the spectrum_quantity_none branch June 22, 2023 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] BSDF constructor inconsistency
1 participant