Classy transfer_model #63

jlashner · 2020-05-20T22:02:11Z

Hi Steven, here's my first shot at a classy transfer model. It's behavior is as follows:

If a 'class_obj' is specified in transfer params, it will attempt to use that class object instead of creating a new one. It will figure out on its own whether or not .compute() has been run by checking to see if the transfer function exists yet.
If no class_obj is specified, a new class object is created. It uses a few default parameters like omega_b, omega_cdm, and h based on the cosmo object, updated with any parameters passed via class_params.

With our modified class code, it is possible to obtain negative T(k), which will break this since you can't take ln(T). If this transfer is only used to calculate the power spectrum, then it should be fine to return ln(|T|), since P(k)~T(k)^2. However if other parts of your code use T(k) (not squared) then this might be a problem. I did a quick scan of hmf and couldn't really find T(k) being used outside of calculating P(k). Do you have an opinion on the best way to handle this? In any case, I added a flip_T transfer option which returns ln(|T|), but we should discuss if there is a better way to handle this.

codecov · 2020-05-20T22:09:45Z

Codecov Report

Merging #63 into master will decrease coverage by 0.51%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   65.41%   64.89%   -0.52%     
==========================================
  Files          22       22              
  Lines        2495     2527      +32     
==========================================
+ Hits         1632     1640       +8     
- Misses        863      887      +24

Impacted Files	Coverage Δ
hmf/density_field/transfer_models.py	`76.88% <25.00%> (-8.61%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef35734...0962687. Read the comment docs.

steven-murray

This all looks really great so far -- it's definitely got the right usage of things.

I suggest adding a test for the new class, to convince ourselves it's doing the right thing. Aim for 100% coverage of this class! I can help with that if you need me to as well.

steven-murray · 2020-05-21T14:53:33Z

hmf/density_field/transfer_models.py

+    import classy
+except ImportError as e:
+    pass
+
 _allfits = ["CAMB", "FromFile", "EH_BAO", "EH_NoBAO", "BBKS", "BondEfs"]


You can add "CLASS" to this list

steven-murray · 2020-05-21T14:56:39Z

hmf/density_field/transfer_models.py

+        super(CLASS, self).__init__(*args, **kwargs)
+        self.spline_fn = None
+
+        if self.params['class_obj'] is not None:


Can you put a check whether the actual class is an instance of classy.Class?

steven-murray · 2020-05-21T15:00:02Z

hmf/density_field/transfer_models.py

+            trans = self.class_obj.get_transfer(output_format='camb')
+            if not trans:
+                self.class_obj.compute()
+                trans = self.class_obj.get_transfer(output_format='camb')


I'm not very familiar with Class, but I feel like there might be a more elegant way to do this. Is there no other way to check if compute has been called other than to get the transfer and check if it's not None? Ideally, you'd have:

if not self.class_obj.condition: self.class_obj.compute() trans = self.class_obj.get_transfer(output_format='camb')

steven-murray · 2020-05-21T15:02:10Z

hmf/density_field/transfer_models.py

+        **class_object:**
+            Custom class object. If not set, a new Class object will be
+            created.
+        **flip_t:**


The transfer function in hmf is only used to get the power spectrum (at this point). I think it's reasonable just to always take the absolute for now. It might be good to post an issue about this though, and we can think about returning the actual transfer function rather than the log. I chose the log because at the time I thought it must always be positive, and the log is easier to take a spline of.

Ah, you'll need to remove flip_t from the docsring as well :-)

steven-murray · 2020-05-21T15:02:28Z

hmf/density_field/transfer_models.py

+                trans = self.class_obj.get_transfer(output_format='camb')
+
+            _k, _t = trans['k (h/Mpc)'], trans['-T_tot/k2']
+            if self.params['flip_T']:


Note point above about using absolute.

steven-murray · 2020-05-21T15:08:33Z

hmf/density_field/transfer_models.py

+try:
+    import classy
+except ImportError as e:
+    pass


Set HAVE_CLASS=False here if it fails, then raise an error in the class __init__ if it's false.

Also, add a line to the setup.cfg in the extras_require section to the effect of

class = classy

steven-murray · 2020-05-21T15:08:59Z

hmf/density_field/transfer_models.py

+    _defaults = {"class_params": None, "class_obj": None,
+                 "flip_T": False}
+
+    def __init__(self, *args, **kwargs):


Here's the place to raise an error if HAVE_CLASS=False

steven-murray · 2020-05-21T15:12:35Z

Also, before we merge, you should:

Add your name to the contributors list (actually, it might already be there)
Add your changes to the CHANGELOG.rst
Install pre-commit and run pre-commit run -a in the top-level of the repo. I think the Linter is failing because of some black formatting issues. This will fix those automatically (most likely).

We'll also have to resolve conflicts with master, but let's do those things first.

jlashner · 2020-05-24T06:59:40Z

Great! Implemented all of your suggestions and changed a few things:

Added a test comparing CLASS and CAMB transfers with default params, making sure they are the same
I removed the flip_T argument, and if T is negative I'm printing a warning and just returning the absolute value. We're a bit concerned if this will work properly when splined, so I think we still might want to implement an option where you don't have to take the log.
Had some flake8 problems about \* characters in the docstring I copied from the other transfer functions. I'm not sure why it didn't like those in the CLASS transfer but is ok with them in the other transfer functions, but I just removed them.

Let me know if there's anything else, thanks!

jlashner · 2020-05-24T19:25:54Z

So I think this is failing my test because class is not installed on the test server. Do you have any idea how to add that? I think it needs to be manually installed. Cobaya-install can also be used to install a suite of cosmological packages which might be easier.

I did run the tests locally and they passed.

steven-murray

OK, this all looks good. To get tests working, you should do two things.

First, make sure the test doesn't run when CLASS is not installed. You can do this by using the pytest.importorskip function as the first thing inside the test function. This means people don't have to have CLASS installed to run the tests, and it will just print out that it got skipped (to remind them that there are more tests they might want to run).

Second, you'll have to edit .github/workflows/test_suite.yaml to add in installation of CLASSY. At around line 70 you should find an "Install" section. Add on the relevant commands to the end of that section (it seems like it's not going to just be pip install classy unfortunately). It's just bash running there, so you can do whatever you need to do (you could also create a new section under "Install" and call it "Install CLASSY").

steven-murray · 2020-05-25T15:23:13Z

hmf/density_field/transfer_models.py

+
+
+class CLASS(FromFile):
+    """


Add an r just before the quotes to be able to use the \*\* notation in the docstring (not that you have to do that).

steven-murray · 2020-05-25T15:23:16Z

hmf/density_field/transfer_models.py

+        **class_object:**
+            Custom class object. If not set, a new Class object will be
+            created.
+        **flip_t:**


Ah, you'll need to remove flip_t from the docsring as well :-)

steven-murray · 2020-07-10T13:43:17Z

Hi @jlashner, did you progress with this? Can I assist in any way?

jlashner · 2020-07-28T18:23:50Z

Hi Steven, sorry for leaving this out for so long. So I stopped working on this because we were running into some consistency issues between our modified CLASS model and the cosmology that's used throughout hmf, which is an astropy cosmology model. It seems to me that in order to be consistent we'd have to make a new cosmology class that wraps our CLASS object and maps all of the used astropy methods to CLASS methods. Do you have any input on whether this is the right thing to do, and if there would be any lingering astropy cosmology dependence in HMF? Thanks!

Anyway, after we figured out that we'd need to put more work into hmf for it to be consistent I decided to move ahead with other parts of the project while we looked into some other options or determined whether the CLASS wrapper was worth implementing in HMF. It looks like the only thing thats left is some minor docstring comments that I can implement. I was also holding off because the CLASS installation is not all that straightforward and I'm not sure the best way to do it through github actions. If you want to take a shot at it that would be super helpful!

steven-murray · 2020-07-28T22:10:55Z

Hi @jlashner, thanks for the update!

How bad is the inconsistency? Is it a fundamental inconsistency between astropy and CLASS, or between your modification of CLASS only? Could we get away with using the code as-is for now, and making an issue that we'll want to someday update the CLASS wrapper?

The CLASS installation is indeed not straightforward. I think I can give it a go soon.

First shot at Classy transfer_model.

3524b9e

steven-murray reviewed May 21, 2020

View reviewed changes

Implemented suggestions and added tests

5057eb9

steven-murray reviewed May 25, 2020

View reviewed changes

timothydmorton mentioned this pull request Jun 8, 2020

CLASS growth factor #71

Open

Implemented docstring comments

0962687

steven-murray changed the base branch from master to dev November 25, 2020 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classy transfer_model #63

Classy transfer_model #63

jlashner commented May 20, 2020

codecov bot commented May 20, 2020 •

edited

Loading

steven-murray left a comment

steven-murray May 21, 2020

steven-murray May 21, 2020

steven-murray May 21, 2020

steven-murray May 21, 2020

steven-murray May 25, 2020

steven-murray May 21, 2020

steven-murray May 21, 2020 •

edited

Loading

steven-murray May 21, 2020

steven-murray commented May 21, 2020

jlashner commented May 24, 2020

jlashner commented May 24, 2020

steven-murray left a comment

steven-murray May 25, 2020

steven-murray May 25, 2020

steven-murray commented Jul 10, 2020

jlashner commented Jul 28, 2020

steven-murray commented Jul 28, 2020

Classy transfer_model #63

Are you sure you want to change the base?

Classy transfer_model #63

Conversation

jlashner commented May 20, 2020

codecov bot commented May 20, 2020 • edited Loading

Codecov Report

steven-murray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-murray May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-murray commented May 21, 2020

jlashner commented May 24, 2020

jlashner commented May 24, 2020

steven-murray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-murray commented Jul 10, 2020

jlashner commented Jul 28, 2020

steven-murray commented Jul 28, 2020

codecov bot commented May 20, 2020 •

edited

Loading

steven-murray May 21, 2020 •

edited

Loading