CobayaSampler / cobaya

Code for Bayesian Analysis
http://cobaya.readthedocs.io/en/latest/
Other
122 stars 125 forks source link

Fixing requisites for bao generic with different kinds of observables #344

Closed Kushallodha closed 6 months ago

Kushallodha commented 6 months ago

Fixing computation of requisites for generic BAO when supplied with measurements_file that contains different kinds of observables at several redshifts. To validate the logic, I tested it on the dummy file in the attachment. The older version incorrectly returns {'angular_diameter_distance': {'z': array([2.34])}, 'Hubble': {'z': array([2.34])}, 'rdrag': None, 'fsigma8': {'z': array([2.34])}}.

z value observable
0.29 1 DV_over_rs
1.31 1 DV_over_rs
1.31 1.5 F_AP
0.5 1.8 DV_over_rs
0.5 1.2 F_AP
0.7 1.1 DV_over_rs
0.7 1.6 F_AP
0.91 1.5 DV_over_rs
0.91 1 F_AP
1.49 1.5 DV_over_rs
2.34 2 DM_over_rs
2.34 5 DH_over_rs
2.34 2 f_sigma8

bao_mean_check.txt

cmbant commented 6 months ago

Thanks, the tests are failing, but probably just a trivial whitespace issue.

cmbant commented 6 months ago

Shouldn't it be something like this?

        if self.has_type:
            for observable in self.data["observable"].unique():
                for req, req_values in theory_reqs[observable].items():
                    if req not in requisites:
                        requisites[req] = req_values
                    else:
                        for k, v in req_values.items():
                            if v is not None:
                                requisites[req][k] = np.unique(
                                    np.concatenate((requisites[req][k], v)))
codecov-commenter commented 6 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ea587ce) 81.78% compared to head (1e32d78) 81.75%.

Files Patch % Lines
cobaya/likelihoods/base_classes/bao.py 62.50% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #344 +/- ## ========================================== - Coverage 81.78% 81.75% -0.03% ========================================== Files 133 133 Lines 11033 11039 +6 ========================================== + Hits 9023 9025 +2 - Misses 2010 2014 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Kushallodha commented 6 months ago

Yes, thank you @cmbant for pointing this out. It needs to be for observable in self.data["observable"].unique() but we still need to check (if isinstance(req_values, dict)) req_values is dict before for loop. Let me know if I missed a better/simplified way to do the same.

        if self.has_type:
            for observable in self.data["observable"].unique():
                for req, req_values in theory_reqs[observable].items():
                    if req not in requisites.keys():
                        requisites[req] = req_values
                    else:
                        if isinstance(req_values, dict):
                            for k, v in req_values.items():
                                if v is not None:
                                    requisites[req][k] = np.unique(np.concatenate((requisites[req][k], v)))

For the time being, all the checks have passed. Please let me know if you prefer all commit history squashed into one. If not, then I will submit the pull request.

cmbant commented 6 months ago

Thanks, why do you need to check it is dict - aren't they all dict? Don't worry about squash, we do that when merging.

Kushallodha commented 6 months ago

I think rdrag's value ( {"rdrag": None}) is not the dict.

cmbant commented 6 months ago

None is caught by the "if v is not None", the whole of {"rdrag":None} is a dict insance?

cmbant commented 6 months ago

(if you have a simple test, it would be good to add it to the unit tests along with the other bao tests)

Kushallodha commented 6 months ago

In case of {"rdrag":None}, req_values is None and for loop crashes before if condition as None can't be iterated over with .items().

I've created a basic test using mock data for mixed observables, similar to 'test_generic_camb`, which requires a mock mean and data. Please let me know if this is okay or if you have something different in mind.

cmbant commented 6 months ago

Thanks, probably just need to update BAO's github_release": to "v2.3" for new data tests to pass.

Kushallodha commented 6 months ago

I am not sure why tests are failing. I did not touch any camb-related files.

cmbant commented 6 months ago

Yeah, weird - I'm sure it's nothing to do with this PR.

cmbant commented 6 months ago

I think it was just a temporary github issue, seems to pass now. Thanks for the PR!