Skip to content

gh-209: implement mgclass#302

Open
zsirap wants to merge 67 commits into
mainfrom
290_mgclass_BZ
Open

gh-209: implement mgclass#302
zsirap wants to merge 67 commits into
mainfrom
290_mgclass_BZ

Conversation

@zsirap
Copy link
Copy Markdown
Collaborator

@zsirap zsirap commented Oct 23, 2025

🚀 Pull Request Checklist

✅ Summary

closes #290

🔄 Changes

  • allows the user to call additional parameters for the different models in MGCLASS
  • allows to import separate CLASS and MGCLASS in the same python script

🛠 How to Test

📝 Documentation

  • This PR updates documentation

🏗 Related Issues

Resolves #290

📸 Screenshots (if applicable)

📌 Additional Notes


✅ PR Checklist for Developers

  • I have titled this PR before merging as "gh-#:", where "#" represents the task it closes
  • I have run locally pre-commit using pre-commit run --all-files
  • I have tested my changes locally
  • No new warnings or errors introduced
  • I have updated documentation (if applicable)
  • My changes do not introduce breaking changes (i.e: the package still gets installed)
  • I have added unit tests (if applicable)
  • I have consistently updated the GitHub information for the project, including milestones, task types, and other relevant details.

✅ PR Checklist for Reviewers

  • The next PR targets the correct branch
  • CI tests have run and passed for the latest commit on the source branch
  • Check that the code can still be installed if new packages are imported
  • If necessary, the notebooks in the playground will be updated in a corresponding follow-up PR
  • Coverage percentage is retained or increased
  • Quality of new/changed code is acceptable
  • Quality of new/changed unit tests is acceptable
  • No data files have been included in the commits
  • Implementation follows the agreed task description point by point
  • Check that there are no No newline at the end of file warnings
  • Check that any added folder/file has been added to the README.md file
  • Check that the implementation follows the contributing guidelines and style choices
  • Check that the documentation has been updated accordantly
  • Check that the corresponding branch has been deleted after merging. If not, delete it

@zsirap zsirap requested a review from gcanasherrera October 23, 2025 01:11
@zsirap
Copy link
Copy Markdown
Collaborator Author

zsirap commented Oct 23, 2025

I am still getting style errors that I don't know the origin (though locally the style test and mglass test pass) if you can help on that @gcanasherrera

update: i see other PRs have some similar issues ... later i might recheck again to see if i manage to solve mine .. for time being i labeled the PR as draft but it can be already reviewed at your or any other reviewer ease

@gcanasherrera
Copy link
Copy Markdown
Member

Make sure this title of this PR is properly named

@gcanasherrera gcanasherrera added background-perturbations Tasks related to Background and Perturbations th-1 labels Oct 23, 2025
@gcanasherrera gcanasherrera added this to the v0.11 milestone Oct 23, 2025
@zsirap zsirap changed the title 290 mgclass bz 290 mgclass integration Oct 23, 2025
@zsirap zsirap marked this pull request as ready for review October 29, 2025 02:24
@zsirap
Copy link
Copy Markdown
Collaborator Author

zsirap commented Oct 29, 2025

not a draft anymore ... it only fails now because MGCLASS is an external code and is not automatically installed ... otherwise it passes locally ...

@gcanasherrera
Copy link
Copy Markdown
Member

Hi @zsirap,

Before I jump into the reviewing and into addressing the issue on how to install MGCLASS into the CI environment, could you please make sure that you are following properly the requests specified in the PR Checklist for Developers?

For instance, you have clicked as done "I have titled this PR before merging as "gh-#:", where "#" represents the task it closes", however you have not done it.

Also, make sure you have a proper picture that you would like to share in your GitHub profile so that the all-contributors bot gives you visibility accordantly (if you want to keep the default GitHub picture, too, it's also fine).

@zsirap zsirap changed the title 290 mgclass integration 290 implement mgclass Oct 29, 2025
@zsirap zsirap changed the title 290 implement mgclass gh-implement mgclass Oct 29, 2025
@zsirap zsirap changed the title gh-implement mgclass gh-209: implement mgclass Oct 29, 2025
@zsirap
Copy link
Copy Markdown
Collaborator Author

zsirap commented Oct 29, 2025

For instance, you have clicked as done "I have titled this PR before merging as "gh-#:", where "#" represents the task it closes", however you have not done it.

ah i thought you meant by this previous comment changing the title to make it more suitable ...

Make sure this title of this PR is properly named

my bad .. ok done now i hope ...

Before I jump into the reviewing

no rush .. at your ease @gcanasherrera ..

if you want to keep the default GitHub picture, too, it's also fine

for the moment ... maybe later

@gcanasherrera
Copy link
Copy Markdown
Member

gcanasherrera commented Oct 29, 2025

Hi @zsirap,

Thanks for this. As you're one of the main users of MGCLASS, I was wondering: why don't you make it pip installable so it is easier for us to install, also in the CI?
CC- @matmartinelli

Copy link
Copy Markdown
Member

@gcanasherrera gcanasherrera left a comment

Choose a reason for hiding this comment

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

Please, adapt the names to MGCLASS instead of CLASS only everywhere

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread cloelib/cosmology/mgclass_cosmology.py Outdated
Comment thread cloelib/cosmology/mgclass_cosmology.py Outdated
Comment thread cloelib/cosmology/mgclass_cosmology.py Outdated
Comment thread cloelib/cosmology/mgclass_cosmology.py Outdated
Comment thread cloelib/cosmology/mgclass_cosmology.py Outdated
Comment thread tests/test_mgclass_cosmology.py
Comment thread tests/test_mgclass_cosmology.py Outdated
@gcanasherrera gcanasherrera marked this pull request as draft April 16, 2026 22:01
@zsirap
Copy link
Copy Markdown
Collaborator Author

zsirap commented May 4, 2026

The closest it can be ready within last or this week deadline ... however couldn't manage but to still need to install mgclass separately in ci.yaml .. solving that in the future requires to separate/rename internal functions completely from class, or fully align with latest version of class but we see the issues when any of the xxclass pip is built on a diff version than plain class ...

It remains to include Ivan's fix I guess after he merges his branch ... note that I was already aware of the bug but was changing in private the lkl in cloelike instead but now will follow his fix though don't know where the fix should be: in the BS, or the emulators or the cloelike but let's suppose that Ivan fix is the best option thus I will implement it ...

Note also that while investigating where the fix should be I spotted some issues in some of the protocols such that I might open an issue to address/discuss them ...

@gcanasherrera gcanasherrera marked this pull request as ready for review May 4, 2026 11:59
@gcanasherrera
Copy link
Copy Markdown
Member

You're assigned to this one @santiagocasas, you're more than a CLASS person than me.

@gcanasherrera
Copy link
Copy Markdown
Member

@zsirap (CC- @santiagocasas):

note how class is now interfacing in the main branch, where it requires an optional linear module to be passed to the non-linear module. This is to allow for cloelike interface. Please, follow the same approach in mgclass

@santiagocasas
Copy link
Copy Markdown
Contributor

@gcanasherrera yes, I am taking a look into this. But regarding your comment on the linear module being passed, I think we should make mgclass and hi_class more modular. We are currently duplicating a lot of boilerplate code for background for instance, and this is not only dangerous but difficult to maintain (cloelib/cosmology/mgclass_cosmology.py).

Let me think about this for a day.

@santiagocasas
Copy link
Copy Markdown
Contributor

thanks @zsirap for your work on this so far

@gcanasherrera gcanasherrera added the priority For PM purposes label May 11, 2026
Copy link
Copy Markdown
Contributor

@santiagocasas santiagocasas left a comment

Choose a reason for hiding this comment

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

all in all, quite close to finishing, but a few things need revision. Especially Protocol adherence and the physical error (double-check!) in Omega_cb and so on. See below! thanks!

Comment thread .github/workflows/ci.yaml
run: |
mamba run -n $ENV_NAME python -m pip install --upgrade pip
mamba run -n $ENV_NAME python -m pip install ".[camb,classy,euclidemu2,baccoemu]"
- name: Install external prereq using pip
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please don't install mgclassy like that. Use the same format as for the other external codes and solve the conflict, so that all codes are there. There is no need to specify the version, this is installed via the pyproject.toml and this is set there already: mgclassy = ["mgclassy==2.9.4.5"]

self.Omega_cdm0 = Omega_cdm0
self.Omega_k0 = Omega_k0
self.As = As
self.ns = ns
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please rebase to latest version of main and add here the new requirement in the Background protocol, the parameter alpha_s. See here:

alpha_s: float = 0.0,

Comment thread cloelib/cosmology/mgclass_cosmology.py Outdated
[
(self.results.Omega_b() + self.results.Omega0_cdm())
* (1 + z) ** 3.0
/ (self.hubble_parameter(z) / self.H0) ** 0.5
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why the square root of E(z)? , it should be (E(z))^2.
If possible, simply use the exposed variables as CLASS does: return self.results.Om_b(zs) + self.results.Om_cdm(zs).
Also, avoid for-loops. These functions are usually broadcastable.

Comment thread tests/test_mgclass_cosmology.py Outdated
return {"Linear": mgclass_lin, "NonLinear": mgclass_non}


@pytest.mark.parametrize("key", ["Linear"])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

change to @pytest.mark.parametrize("key", ["Linear", "NonLinear"]) so you also test the NonLinear adherence to the protocol

)

return D_z_k

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for MGCLASSNonLinearPerturbations to adhere to the Perturbations protocol, it needs to have a growth_rate, see here:

def growth_rate(self) -> np.ndarray:

Tests were passing because you were not testing protocol adherence in NonLinear. See my comment in tests/test_mgclass_cosmology.py


# add neutrinos:
mgclass_background_instance.interface_args["MGCLASSparams"]["N_ncdm"] = 1
mgclass_background_instance.interface_args["MGCLASSparams"]["m_ncdm"] = 0.2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

using this here, while leaving these other parameters as:
mgclass_background_instance.mnu == 0.0 mgclass_background_instance.N_mnu == 0
is inconsistent, in case someone uses another parametrization. Tests might fail or pass for the wrong reason

@gcanasherrera
Copy link
Copy Markdown
Member

@zsirap do you have any time scale for addressing these comments?

@zsirap
Copy link
Copy Markdown
Collaborator Author

zsirap commented May 19, 2026

@zsirap do you have any time scale for addressing these comments?

was in a workshop ... will start addressing them from today ... thanks for reminding me and thanks to @santiagocasas for the extra review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

background-perturbations Tasks related to Background and Perturbations priority For PM purposes th-1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement MGCLASS

4 participants