Serious bug: Problems in metadata extraction functions result in severe wrong behaviour when constructing likelihoods from sacc and factories!

[arthurmloureiro]

Describe the bug
Following the conversation on the Firecrown Slack Channel: what defines what in the sacc is linked to what in the factory yaml (the thing set to type_source: default) is the Nz type that firecrown reads from the metadata. However, nothing in the sacc tells firecrown when using extract_all_harmonic_data or similar functions to fill that metadata information with anything else but default see here:

firecrown/firecrown/metadata_functions.py

Line 254 in 0216ee3

InferredGalaxyZDist(

and here

firecrown/firecrown/metadata_types.py

Line 336 in 0216ee3

type_source: TypeSource = TypeSource.DEFAULT

so even though the SACC has different tracers, the metadata reader from firecrown will always consider everything the same.

In other words, since nothing from the functions that are used to extract the metadata from the SACC is used to differentiate different type_source when constructing the Nz part of the metadata, all tracers of the same type are consider the same.

This is one problem.

The more serious problem is that when providing the likelihood builder from yaml factories and sacc metadata, NOTHING breaks to tell the user that there are more type_sources in the yaml factories than type_sources in the sacc metadata.

To Reproduce
When reading a sacc that contains more than one tracer of the same type:

for n, t in sacc_data.tracers.items():
    print(t.name, t.quantity, type(t))

resulting in:

gs0 generic <class 'sacc.tracers.NZTracer'>
gs1 generic <class 'sacc.tracers.NZTracer'>
gs2 generic <class 'sacc.tracers.NZTracer'>
gs3 generic <class 'sacc.tracers.NZTracer'>
gs4 generic <class 'sacc.tracers.NZTracer'>
gs5 generic <class 'sacc.tracers.NZTracer'>
gs6 generic <class 'sacc.tracers.NZTracer'>
gs7 generic <class 'sacc.tracers.NZTracer'>
gs8 generic <class 'sacc.tracers.NZTracer'>
gs9 generic <class 'sacc.tracers.NZTracer'>
gs10 generic <class 'sacc.tracers.NZTracer'>
g0 generic <class 'sacc.tracers.NZTracer'>
s0 generic <class 'sacc.tracers.NZTracer'>
g1 generic <class 'sacc.tracers.NZTracer'>
s1 generic <class 'sacc.tracers.NZTracer'>
g2 generic <class 'sacc.tracers.NZTracer'>
s2 generic <class 'sacc.tracers.NZTracer'>
g3 generic <class 'sacc.tracers.NZTracer'>
s3 generic <class 'sacc.tracers.NZTracer'>
g4 generic <class 'sacc.tracers.NZTracer'>
s4 generic <class 'sacc.tracers.NZTracer'>
g5 generic <class 'sacc.tracers.NZTracer'>
s5 generic <class 'sacc.tracers.NZTracer'>

following the tutorial to extract the metadata from the sacc in order to construct a firecrown likelihood using the facties, we can use this

two_point_reals = extract_all_harmonic_data(sacc_data)

The function extract_all_harmonic_data makes no distinction in the Nz metadata for g tracers or gs tracers, they are both measurements={<Galaxies.COUNTS: '1'>}, type_source='default')

Now if I use the following yaml of factories:

correlation_space: harmonic
weak_lensing_factories:
  - type_source: default
    per_bin_systematics:
    - type: MultiplicativeShearBiasFactory
    - type: PhotoZShiftFactory
    global_systematics:
    - type: LinearAlignmentSystematicFactory
      alphag: 1.0
number_counts_factories:
  - type_source: default
    per_bin_systematics:
    - type: PhotoZShiftFactory
    global_systematics: []
  - type_source: default-spec
    per_bin_systematics:
    - type: PhotoZShiftandStretchFactory
    - type: MagnificationBiasSystematicFactory
    global_systematics: []

I can still construct the likelihood using:

tp_factory = base_model_from_yaml(TwoPointFactory, two_point_yaml)
two_points_ready = TwoPoint.from_measurement(two_point_reals, tp_factory)
likelihood_ready = ConstGaussian.create_ready(
    two_points_ready, sacc_data.covariance.dense
)

Nothing breaks from the fact that there are more type_sources in the factory yaml than in the metadata.

Expected behavior

1. A way to tell the extract_metadata functions about different tracers of the same type of measurement:
   measurements={<Galaxies.COUNTS: '1'>}, type_source='red') and measurements={<Galaxies.COUNTS: '1'>}, type_source='blue') for example
2. The TwoPoint.from_measurement(two_point_reals, tp_factory) should break if more type_sources are requested than present in the metadata.

Observed behavior
Described above

Output
Here's the output from printing the default values of systematic parameters from the example above:

{'alphaz': 0.0,
 's2_delta_z': 0.0,
 'g2_bias': 1.5,
 's2_mult_bias': 1.0,
 'g1_bias': 1.5,
 'gs2_delta_z': 0.0,
 's4_mult_bias': 1.0,
 'gs5_delta_z': 0.0,
 'gs2_bias': 1.5,
 'gs9_bias': 1.5,
 'g0_bias': 1.5,
 'gs5_bias': 1.5,
 's1_mult_bias': 1.0,
 'gs8_delta_z': 0.0,
 'gs7_delta_z': 0.0,
 'ia_bias': 0.5,
 's4_delta_z': 0.0,
 'gs6_bias': 1.5,
 's1_delta_z': 0.0,
 'g3_delta_z': 0.0,
 'gs0_bias': 1.5,
 'gs8_bias': 1.5,
 'z_piv': 0.5,
 'gs6_delta_z': 0.0,
 'gs0_delta_z': 0.0,
 's3_mult_bias': 1.0,
 'g4_bias': 1.5,
 'gs7_bias': 1.5,
 's3_delta_z': 0.0,
 'gs4_bias': 1.5,
 'g0_delta_z': 0.0,
 's5_delta_z': 0.0,
 'gs1_delta_z': 0.0,
 'gs4_delta_z': 0.0,
 'g5_delta_z': 0.0,
 's5_mult_bias': 1.0,
 'g5_bias': 1.5,
 's0_mult_bias': 1.0,
 'gs3_delta_z': 0.0,
 'gs10_delta_z': 0.0,
 'gs9_delta_z': 0.0,
 'gs1_bias': 1.5,
 'gs10_bias': 1.5,
 'g4_delta_z': 0.0,
 'g3_bias': 1.5,
 'g1_delta_z': 0.0,
 's0_delta_z': 0.0,
 'g2_delta_z': 0.0,
 'gs3_bias': 1.5}

note that nothing is releated to the MagnificationBiasSystematicFactory or the PhotoZShiftandStretchFactory even though these were present in the yaml factory and no errors were thrown.

Configuration:
Please include all relevant information about your configuration:

• Firecrown version: '1.12.0a0'

Additional context
Add any other context about the problem here.

[vitenti]

Hi @arthurmloureiro, thanks for the report. After looking into it, this doesn't actually seem to be a bug in Firecrown. The behavior you're seeing, where all tracers are assigned type_source=DEFAULT, is expected given the current metadata in SACC.

SACC doesn't store a TypeSource per tracer, so Firecrown has no way to distinguish them automatically. That's why extra type_sources in the YAML factories don't match anything in the extracted metadata.

This seems more like a feature request: Firecrown could allow users to supply a mapping from tracer names to type_source when extracting metadata, or SACC itself could be extended to carry this information.

A possible workaround in the meantime is to provide an explicit mapping when calling extract_all_harmonic_data, e.g.:

two_point_reals = extract_all_harmonic_data(
    sacc_data,
    tracer_to_type_source={
        'g0': 'counts-red',
        'g1': 'counts-blue',
        's0': 'shear-red',
        ...
    }
)

This will let your YAML factories apply systematics correctly.

Note that, this can already be handled by the user. One can call extract_all_harmonic_data and then modify the TypeSources manually before feeding the data to the factories.

Naturally, we could provide an automated solution (as above), but this requires discussion: if the possible use cases are too broad, adding full automation could make the system complex. In that case, handling it case by case might remain the best approach.

---

The more serious problem is that when providing the likelihood builder from yaml factories and sacc metadata, NOTHING breaks to tell the user that there are more type_sources in the yaml factories than type_sources in the sacc metadata.

Regarding the concern about factories associated with TypeSources that are not present in the metadata: the current design intentionally does not enforce a global check.

Factories define rules for constructing TwoPoint objects for specific TypeSources. These rules are applied per object. Because TwoPoints are constructed one by one, detecting whether a factory’s rules were never applied would require a global view of all TwoPoints being created.

In practice, this may not be desired: users may define a set of factories to apply generally, knowing that in some cases certain factories will not match any metadata.

[arthurmloureiro]

Regarding the concern about factories associated with TypeSources that are not present in the metadata: the current design intentionally does not enforce a global check.

Factories define rules for constructing TwoPoint objects for specific TypeSources. These rules are applied per object. Because TwoPoints are constructed one by one, detecting whether a factory’s rules were never applied would require a global view of all TwoPoints being created.

In practice, this may not be desired: users may define a set of factories to apply generally, knowing that in some cases certain factories will not match any metadata.

Hi @vitenti

So this particular problem has already caused some trouble when using firecrown in forecasts and building likelihoods for data analysis. Me, @bruno-moraes , and @iagolops had an undetectable bug from the factories that took ages to catch because of factories declared in the likelihood yaml but not present in the sacc (a very very very very likely mistake). In the particular example above, had I not found that it was weird that nothing broke, we could have ended up with a production likelihood that was ignoring half of the factories without being able to catch it. Luckily, I decided to check the names of the systematics parameters and noticed that many were missing.

I am not particularly familiar with the libraries used in firecrown to build objects from a yaml file so it's not obvious to me why implementing a check that there are the same number of required factories as factories present in the yaml constructer requires a global view (and why that is complicated).