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

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

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

Kushallodha
Copy link
Contributor

@Kushallodha Kushallodha commented Dec 30, 2023

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

@Kushallodha Kushallodha marked this pull request as draft December 30, 2023 17:28
@cmbant
Copy link
Collaborator

cmbant commented Jan 3, 2024

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

@cmbant
Copy link
Collaborator

cmbant commented Jan 5, 2024

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
Copy link

codecov-commenter commented Jan 7, 2024

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 ⚠️

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kushallodha
Copy link
Contributor Author

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
Copy link
Collaborator

cmbant commented Jan 7, 2024

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
Copy link
Contributor Author

Kushallodha commented Jan 8, 2024

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

@Kushallodha Kushallodha marked this pull request as ready for review January 8, 2024 02:28
@cmbant
Copy link
Collaborator

cmbant commented Jan 8, 2024

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

@cmbant
Copy link
Collaborator

cmbant commented Jan 8, 2024

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

…g a bug in bao base_class for inversion of covariance matrix with only one observable.
@Kushallodha
Copy link
Contributor Author

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
Copy link
Collaborator

cmbant commented Jan 9, 2024

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

@Kushallodha
Copy link
Contributor Author

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

@cmbant
Copy link
Collaborator

cmbant commented Jan 9, 2024

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

@cmbant
Copy link
Collaborator

cmbant commented Jan 9, 2024

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

@cmbant cmbant merged commit 62e1ca8 into CobayaSampler:master Jan 9, 2024
1 check failed
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.

None yet

3 participants