gh-209: implement mgclass - #302
Conversation
|
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 |
|
Make sure this title of this PR is properly named |
|
not a draft anymore ... it only fails now because MGCLASS is an external code and is not automatically installed ... otherwise it passes locally ... |
|
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). |
ah i thought you meant by this previous comment changing the title to make it more suitable ...
my bad .. ok done now i hope ...
no rush .. at your ease @gcanasherrera ..
for the moment ... maybe later |
|
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? |
gcanasherrera
left a comment
There was a problem hiding this comment.
Please, adapt the names to MGCLASS instead of CLASS only everywhere
|
You're assigned to this one @santiagocasas, you're more than a CLASS person than me. |
|
@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 |
|
@gcanasherrera yes, I am taking a look into this. But regarding your comment on the linear module being passed, I think we should make Let me think about this for a day. |
|
thanks @zsirap for your work on this so far |
santiagocasas
left a comment
There was a problem hiding this comment.
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!
| 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 |
There was a problem hiding this comment.
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"]
There was a problem hiding this comment.
I agree but for a reason not yet known, mgclass is not installing if i use the same format as the others (I might give it a try again to see if it works again but for the moment if there is urgency we can merge it like that)
a general comment mentioned before and related to this and subsequent comments is that there is a new version of MGCLASS, not released yet, that will be built above a newer or the latest version of CLASS ... but here we started to implement MGCLASS in CLOE as it is now based on an older version of CLASS because also we wanted to be quick and pragmatic and we wanted it to not install above CLASS like other patches do ... and so in trying to do so, one thing led to another such as had to make a pip installable version of what we have now ...
There was a problem hiding this comment.
I will comment on this below
| self.Omega_cdm0 = Omega_cdm0 | ||
| self.Omega_k0 = Omega_k0 | ||
| self.As = As | ||
| self.ns = ns |
There was a problem hiding this comment.
please rebase to latest version of main and add here the new requirement in the Background protocol, the parameter alpha_s. See here:
| [ | ||
| (self.results.Omega_b() + self.results.Omega0_cdm()) | ||
| * (1 + z) ** 3.0 | ||
| / (self.hubble_parameter(z) / self.H0) ** 0.5 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes you are right about Ez ... it must be that I wanted to put E(z) value here by mistake ... fixed
Also, avoid for-loops. These functions are usually broadcastable.
yes but they are not broadcastable in this version of CLASS
| return {"Linear": mgclass_lin, "NonLinear": mgclass_non} | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("key", ["Linear"]) |
There was a problem hiding this comment.
change to @pytest.mark.parametrize("key", ["Linear", "NonLinear"]) so you also test the NonLinear adherence to the protocol
There was a problem hiding this comment.
fixed and fixed everything related to this change
| ) | ||
|
|
||
| return D_z_k | ||
|
|
There was a problem hiding this comment.
for MGCLASSNonLinearPerturbations to adhere to the Perturbations protocol, it needs to have a growth_rate, see here:
cloelib/cloelib/cosmology/class_cosmology.py
Line 578 in c33e526
Tests were passing because you were not testing protocol adherence in NonLinear. See my comment in
tests/test_mgclass_cosmology.py
There was a problem hiding this comment.
fixed as I replied to a previous comment
|
|
||
| # add neutrinos: | ||
| mgclass_background_instance.interface_args["MGCLASSparams"]["N_ncdm"] = 1 | ||
| mgclass_background_instance.interface_args["MGCLASSparams"]["m_ncdm"] = 0.2 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe but I think I followed the same as it is implemented in the plain CLASS test .. so should we change here and there ?
|
@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 |
|
@gcanasherrera @zsirap very busy week unfortunately, but I will look into this in between traveling, hopefully by end of the week! |
|
@zsirap thanks for your patience with this and apologies for the late reply. I have a few comments still for the PR, some of them can be solved now, some of them I have patched myself and some require a larger coordination with the other classy-related EBS codes. |
|
@zsirap I have patched the .github/workflows/ci.yaml so that: @zsirap what is the repo for mgclassy? I would recommend that in the future or current mgclassy, this is fixed by: if you point me to it, I might even make a PR myself there, once I have some time. |
|
@zsirap there was an annoying print coming directly from the C implementation of MGCLASS. This is not good for codes that do CI, testing or when we have to run cloelib/like in production environments.
In the future, @zsirap , please implement a verbosity parameter for MGCLASSY. |
santiagocasas
left a comment
There was a problem hiding this comment.
@zsirap please check that my patches work on your end. Install the cloe-org-environment and install mgclassy with the pip install of cloelib.
If this works, make sure the tests pass and that you agree with my patches.
One more thing, tests currently use: mg_E11=0 and mg_E22=0, you might want to test also some MG functionality.
Once this is checked, ready to merge!
Thanks for your work!
santiagocasas
left a comment
There was a problem hiding this comment.
Tests are passing locally and on the CI with the new patches. After the general update of cloe-org-environments, this can be merged,
|
@gcanasherrera I think you can now safely remove your changes suggestions or click on approve. I will merge after the update to the cloe-org-environments. |
Thank you actually @santiagocasas !! great fix and effort to bring this PR to completion ... Wanted but haven't had time (on the move the past week) to thoroughly check or reply to some of your comments or try to implement others you suggested ... hopefully in the coming days (from my side only since summer break is starting so everything off for the next month) ... But let's not delay further the merge of this PR and merge it as you suggested ... Anything I want to reply, comment or implement, i can open an issue for that later ... Thank you again :) |
|
@gcanasherrera since #566 was merged and passed, I did one final commit here to double check. This should pass and can be merged now. |
🚀 Pull Request Checklist
✅ Summary
closes #290
🔄 Changes
🛠 How to Test
📝 Documentation
🏗 Related Issues
Resolves #290
📸 Screenshots (if applicable)
📌 Additional Notes
✅ PR Checklist for Developers
pre-commit run --all-files✅ PR Checklist for Reviewers
playgroundwill be updated in a corresponding follow-up PRNo newline at the end of filewarningsREADME.mdfile