Skip to content

Conversation

@jokasimr
Copy link
Contributor

@jokasimr jokasimr commented Jan 6, 2026

These test files are useful in CodeShelf

@jokasimr jokasimr requested a review from jl-wynen January 6, 2026 13:37
Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these files be loaded with ESSreflectometry? Do you have an example usage?

)


def estia_mcstas_nexus_example(name):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it odd that this returns either a path or a list of paths. Can you split this into two functions? (I don't see a reason for dynamically deciding which files to get based on a string. The user probably hard codes the string anyway and so they can just as well call a different function.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it is so that the function matches how estia_mcstas_example works. That function is implemented this way because it is convenient (I found it convenient), this test data is not user facing functionality.

Do you have a different suggestion for how the function should work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a different suggestion for how the function should work?

Yes, split it in two (both of them).
The problem is that foo(estia_mcstas_nexus_example(name)) requires that foo can handle both a single path and a list of paths. Here, foo represents not just a function but any expression. E.g., in your example, this

runs = {
    path.stem: {
        Filename[SampleRun]: path
    }
    for path in estia_mcstas_nexus_example('Ni/Ti-multilayer')
}

will fail to pass type checks.

You can argue that you don't care about type checking. But this still increases mental overhead for the reader. You have to understand in what cases the function returns what type. Compare that to estia_mcstas_nexus_ni_ti_multilayer_examples where the name already indicates that there is more than one.

this test data is not user facing functionality.

But it is in a public module. So we are advertising it as a feature of the package. But that argument in general is bad, IMHO. We should strive for the same quality in internal code as user-facing code.

Copy link
Contributor Author

@jokasimr jokasimr Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is in a public module. So we are advertising it as a feature of the package. But that argument in general is bad, IMHO. We should strive for the same quality in internal code as user-facing code.

The argument is not that quality can be bad in non-user facing code, it is that quality means different things in different context.

Your suggestion of splitting the function is fine to me, but will lead to large import statements like

from ess.estia.data import estia_mcstas_reference_example, estia_mcstas_ni_ti_multilayer_examples, estia_mcstas_Si_examples, estia_nexus_SiO2_examples

instead of just

from ess.estia.data import estia_mcstas_examples

and that is also a bit hard to read.

So I'd suggest o only split into two functions: estia_mcstas_reference_example() and estia_mcstas_sample_examples(name).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just import from ess.estia import data [as estia_data].

So I'd suggest o only split into two functions: estia_mcstas_reference_example() and estia_mcstas_sample_examples(name).

Fine by me if the latter always returns a list.

Copy link
Contributor Author

@jokasimr jokasimr Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of splitting the function I'll just make it return a length 1 list when called with 'reference'

@jokasimr
Copy link
Contributor Author

jokasimr commented Jan 7, 2026

Can these files be loaded with ESSreflectometry? Do you have an example usage?

Yes they can be.

from ess.estia.data import estia_mcstas_nexus_example, estia_tof_lookup_table
from ess.estia import EstiaWorkflow
from ess.estia.corrections import default_corrections
from ess.reflectometry.corrections import correct_by_proton_current

from ess.reflectometry.types import *
from ess.reflectometry import supermirror
from ess.reflectometry.tools import batch_compute


wf = EstiaWorkflow()
wf[Filename[ReferenceRun]] = estia_mcstas_nexus_example('reference')
wf[YIndexLimits] = sc.scalar(35), sc.scalar(64)
wf[ZIndexLimits] = sc.scalar(0), sc.scalar(48 * 32)
wf[BeamDivergenceLimits] = sc.scalar(-0.75, unit='deg'), sc.scalar(0.75, unit='deg')

# Don't apply proton current correction, because we don't have proton current values
wf[CorrectionsToApply] = default_corrections - {correct_by_proton_current}

wf[supermirror.MValue] = sc.scalar(5, unit=sc.units.dimensionless)
wf[supermirror.CriticalEdge] = sc.scalar(float('inf'), unit='1/angstrom')
wf[supermirror.Alpha] = sc.scalar(0.25 / 0.088, unit=sc.units.angstrom)

wf[DetectorSpatialResolution[RunType]] = 0.0025 * sc.units.m

# Configure the binning of intermediate and final results:
wf[WavelengthBins] = sc.geomspace('wavelength', 3.5, 12, 2001, unit='angstrom')
wf[QBins] = sc.geomspace('Q', 0.01, 0.5, 401, unit='1/angstrom')

wf[TimeOfFlightLookupTableFilename] = estia_tof_lookup_table()


wf[ProtonCurrent[SampleRun]] = sc.DataArray(
    sc.array(dims=('time',), values=[]),
    coords={'time': sc.array(dims=('time',), values=[], unit='s')})
wf[ProtonCurrent[ReferenceRun]] = sc.DataArray(
    sc.array(dims=('time',), values=[]),
    coords={'time': sc.array(dims=('time',), values=[], unit='s')})

runs = {
    path.stem: {
        Filename[SampleRun]: path
    }
    for path in estia_mcstas_nexus_example('Ni/Ti-multilayer')
}

batch_compute(wf, runs, ReflectivityOverQ).hist().plot(norm='log', vmin=1e-7)
Figure 1(75)

(ProtonCurrent needs to be overridden because it's missing in the files. DetectorSpatialResolution should be a default setting, because it does not change, but currently it is not.)

@jokasimr jokasimr requested a review from jl-wynen January 9, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants