Loading the configuration outside of the functions #67 #69

bazkiaei · 2019-04-09T02:01:27Z

The goal of this PR is to make the code not to load the configuration more than once.

codecov · 2019-04-09T02:04:11Z

Codecov Report

Merging #69 into develop will increase coverage by 0.11%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop      #69      +/-   ##
===========================================
+ Coverage     99.3%   99.42%   +0.11%     
===========================================
  Files            5        5              
  Lines          433      520      +87     
===========================================
+ Hits           430      517      +87     
  Misses           3        3

Impacted Files	Coverage Δ
mocks/tests/conftest.py	`100% <100%> (ø)`	⬆️
mocks/tests/test_mocks.py	`99.22% <100%> (+0.22%)`	⬆️
mocks/tests/utils/test_utils.py	`100% <100%> (ø)`	⬆️
mocks/mocks.py	`99.5% <100%> (+0.07%)`	⬆️

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 170341b...3679f80. Read the comment docs.

bazkiaei · 2019-04-09T02:03:07Z

mocks/mocks.py

@@ -21,12 +21,17 @@
 from mocks.utils import load_yaml_config


+def load_configuration(config_location='config_example.yaml'):


Based on Anthony's advice, I tried to create a dictionary and pass it through other functions instead of kwargs.
So the user should load the configuration first and pass it through other functions.
I hope this is what I was meant to do.

This function is pointless, the effect of having this is just to create an alternative name for mocks.utils.yaml_load_config. You could achieve exactly the same effect with from mock.utils import load_yaml_config as load_configuration, but there's no advantage in doing that, either.

What I suggested was creating a main function that would load the configuration and then call all of the other functions. That would be useful, because then the user would be able to create simulated images from simulation data without having to work out what order to call all of these functions in, or write their own code to so.

The reason why we originally suggesting splitting up the one big function you started with into a series of smaller, self contained functions wasn't that we thought that there shouldn't be a function that does the whole thing. The reason for splitting it up into multiple functions was to make the code easier to understand, test, maintain and reuse. There should still be a function that does the whole thing, it's just that instead of a great big block of code it should consist mostly of calls to the other functions.

There are some inconsistencies among the functions of mocks.py. For instance functions scale_light_by_distance and prepare_mocks both use the distance to the galaxy to scale its light.
The first step to create the main function is making all functions consistent.

After making the functions consistent, the jupyter-notebook examples should be updated as the base for the main function that calls other functions and does all the steps to create the mock image.
These notebooks could be used to create the main function.

bazkiaei · 2019-04-09T02:05:48Z

mocks/tests/conftest.py

+    config['Omega_Lambda'] = 0.727
+    config['Omega_0'] = 0.273
+    config['redshift'] = 0
+    return config


I did not want to load the configuration file for tests other than the one which is testing the mocks.load_configuration so I created this fixture. It is too long and that makes me think maybe I should try something else, so I will appreciate any advice about it.

Why not use configuration files for all the tests? It would be simpler than this.

What you do is have a test fixture that loads the config and returns it (either here or in tests/test_mocks.py), then have a test use the fixture and check that it loaded OK, e.g.

from mock.utils import load_yaml_config @pytest.fixture(scope='package') def configuration(): return load_yaml_config()

and

def test_load_configuration(configuration): assert configuration['data_path'] == 'sim_data/cl19.fits' assert configuration['galaxy_distance'] == 10. assert isinstance(configuration, dict)

The first test to use the configuration fixture will trigger the call to load_yaml_config, and if that fails with an exception then the exception will be passed up to the test, and it will fail (as it should). If it passes then the rest of the tests can run using the same configuration fixture to access the test config.

I applied your suggestion, but is that alright that the function utils.load_yaml_config is tested twice? Once here and another time in test_utils.py

bazkiaei · 2019-04-09T23:13:56Z

@lspitler @AnthonyHorton @wtgee
Ready for the review.

AnthonyHorton

OK, seems like I didn't successful explain my suggestion about where to load the configuration. See comments.

AnthonyHorton · 2019-04-10T06:16:09Z

mocks/mocks.py

@@ -21,12 +21,17 @@
 from mocks.utils import load_yaml_config


+def load_configuration(config_location='config_example.yaml'):


This function is pointless, the effect of having this is just to create an alternative name for mocks.utils.yaml_load_config. You could achieve exactly the same effect with from mock.utils import load_yaml_config as load_configuration, but there's no advantage in doing that, either.

What I suggested was creating a main function that would load the configuration and then call all of the other functions. That would be useful, because then the user would be able to create simulated images from simulation data without having to work out what order to call all of these functions in, or write their own code to so.

The reason why we originally suggesting splitting up the one big function you started with into a series of smaller, self contained functions wasn't that we thought that there shouldn't be a function that does the whole thing. The reason for splitting it up into multiple functions was to make the code easier to understand, test, maintain and reuse. There should still be a function that does the whole thing, it's just that instead of a great big block of code it should consist mostly of calls to the other functions.

mocks/mocks.py

AnthonyHorton · 2019-04-10T06:43:43Z

mocks/tests/conftest.py

+    config['Omega_Lambda'] = 0.727
+    config['Omega_0'] = 0.273
+    config['redshift'] = 0
+    return config


Why not use configuration files for all the tests? It would be simpler than this.

What you do is have a test fixture that loads the config and returns it (either here or in tests/test_mocks.py), then have a test use the fixture and check that it loaded OK, e.g.

from mock.utils import load_yaml_config @pytest.fixture(scope='package') def configuration(): return load_yaml_config()

and

def test_load_configuration(configuration): assert configuration['data_path'] == 'sim_data/cl19.fits' assert configuration['galaxy_distance'] == 10. assert isinstance(configuration, dict)

The first test to use the configuration fixture will trigger the call to load_yaml_config, and if that fails with an exception then the exception will be passed up to the test, and it will fail (as it should). If it passes then the rest of the tests can run using the same configuration fixture to access the test config.

mocks/tests/test_mocks.py

mocks/tests/conftest.py

bazkiaei · 2019-04-17T01:23:45Z

@AnthonyHorton
Ready for review.

AnthonyHorton

A good step in right direction, but use of config versus function arguments is still patchy, and you have parsing of parameters spread around several functions instead of in one parse_config function.

mocks/mocks.py

AnthonyHorton · 2019-04-17T02:05:23Z

mocks/mocks.py

@@ -108,26 +110,31 @@ def scale_light_by_distance(particle_positions,
    return particle_positions.value, particle_values


-def prepare_mocks(config_location='config_example.yaml',
+def prepare_mocks(config,


This function is doing 4 unrelated things. I would suggest splitting this into at least three functions.

The first function fully parses the config, i.e. loads the config file with mocks.utils.load_yaml_config, then goes through all the parameters (not just galaxy_coordinates & observation_time as now) and applies any keyword arguments that override the config file values, gets the remaining values from the config file, adds default values for any parameters that are missing, and does any required unit conversions, etc. This function returns the fully populated, validated config dict for all the other functions to use. The other functions then don't have to do any fiddling around with overriding values, default values, unit conversions, etc.

The second function takes the config as its only argument, loads the simulation data, and returns it. This would be the place to do 32 bit to 64 bit float type conversion, when required.

The calls to compute_total_mass and compute_apparent_ABmag also don't belong here. You could create a 3rd function that calls them, using values from config, and returns the results, or you could do those calls in the main function.

The general principle here is that a function should do one clearly defined thing, the thing that the function name implies that it does. Adding other, unrelated things to a function makes it much harder to understand the software.

…e changed

AnthonyHorton · 2019-04-17T04:00:49Z

mocks/mocks.py

+        kwargs.get('target_galaxy_comoving_depth',
+                   config['target_galaxy_comoving_depth'])
+    config['target_galaxy_comoving_depth'] = ensure_unit(
+        config['target_galaxy_comoving_depth'], u.Mpc)


Don't do all this stuff in this function, it doesn't belong here. Do it in a parse_config function.

@AnthonyHorton
Yes, I will. I just referenced your previous comment in another issue to start creating some functions, including parse_config, in another PR.

This is just a clue for me for creating the parse_config function.

Why not just put it in the right place to begin with? It's not off topic for this PR, which is about improving config loading. Seems to just be creating more work to write the code in one place in one PR, then immediately move it somewhere else in another one. You'll have to modify a bunch of tests twice if you do that.

This is all redundant now that it's being done in parse_config. You should remove it from here.

bazkiaei

@AnthonyHorton
The function parse_config and its tests are created. But it is not used by any other function yet.

bazkiaei · 2019-04-18T06:45:28Z

config_directory/config_example_old.yaml

@@ -0,0 +1,49 @@
+# It is here as an example, and if the user wants to change the default


After creating the parse_config function, more cosmological parameters are added to config_examle.yaml and since the new function should be tested in different ways, the added cosmological parameters are different to the default cosmology, WMAP9. These different parameters cause changes in the result of many functions in test_mocks.py. To avoid huge changes in the tests, I added this file with the same values that config_example.yaml used to have.
However, I am not very happy with the name that I have chosen for it, maybe config_old_example.yaml makes more sense.

I don't think this is needed given my comments below.

bazkiaei · 2019-04-18T06:46:15Z

config_directory/config_no_cosmology_example.yaml

+sim_pc_pixel: 170.
+
+# The baryonic mass of simulation particles (1e5 * Msol/h).
+particle_baryonic_mass_sim: 9.6031355


Oh, I forgot it again!

bazkiaei · 2019-04-18T06:54:03Z

config_directory/config_no_cosmology_example.yaml

@@ -0,0 +1,43 @@
+# It is here as an example, and if the user wants to change the default


This file is added for testing the parse_config file when there is no cosmological parameter in the configuration file provided. The function is supposed to use the default cosmology in that case.

I don't think this is needed given my comments below.

bazkiaei · 2019-04-18T06:54:59Z

config_directory/config_none_value_example.yaml

+T_CMB0:
+Neff:
+m_nu:
+redshift:


bazkiaei · 2019-04-18T06:56:39Z

config_directory/config_none_value_example.yaml

@@ -0,0 +1,54 @@
+# It is here as an example, and if the user wants to change the default


This file has been added to test the capability of the parse_config function when there are defined cosmological parameters in the configuration file but they have no value.

I approve of the thoroughness, but I'm not sure this is entirely necessary. Seems like a strange thing for the user to do.

mocks/mocks.py

bazkiaei · 2019-04-19T02:51:32Z

mocks/mocks.py

+    try:
+        assert config['Omega_b']
+    except AssertionError:
+        config['Omega_b'] = default_cosmology.Ob0


To make the code works for the Dark Matter only simulations, I think maybe I should change the lines that are parsing baryonic matter density as follow:

try: config['Omega_b'] = kwargs.get('Omega_b', config['Omega_b']) except KeyError: config['Omega_b'] = default_cosmology.Ob0 try: assert config['Omega_b'] except AssertionError: if config['Omega_b'] is not 0: config['Omega_b'] = default_cosmology.Ob0

As a general point, if you want to distinguish between something being None, or 0, or some other value which evaluates to False (e.g. an empty list) then you can test for those directly, e.g. if config['Omega_b'] is None: or if config['Omega_b'] == 0:, etc.

As I noted in comments above I really don't think you need to allow for keys without values in the config file so you don't need this added complexity at all.

AnthonyHorton

Part of the reason for parsing the configuration in a dedicated function is to ensure all the other functions use a consistent set of configuration parameters. Consequently you should remove the option to override configuration parameters with keyword argument from the other functions. parse_config should be only place to do that.

Also, nested .get() is a better way to implement the 'keyword arg, or config file, or default value' pattern than the forced exceptions you are using.

You can also simplify things by dropping the requirement to be able to have keys in the config file with no value. I don't know why you would need to be able to do that.

AnthonyHorton · 2019-04-26T01:28:38Z

config_directory/config_example.yaml

 # The resolution of the simulation(parsec / pixel).
-sim_pc_pixel: 170 
+sim_pc_pixel: 170

 # The baryonic mass of simulation particles (1e5 * Msol/h).
 particle_baryonic_mass_sim: 9.6031355

 # Cosmological parameters.


Might include a comment about which cosmology these example parameters correspond too, e.g. WMAP9.

Some of them are the parameters that have been used in the simulation and I just have chosen the others randomly. I intentionally wanted them to be different from WMAP9 for tests purposes.

Different from WMAP9 for tests purposes

that sounds like a test of astropy.cosmology not huntsman-mocks, which shouldn't care about what cosmology is needed.

Are you sure you need this test?

AnthonyHorton · 2019-04-26T01:32:07Z

config_directory/config_none_value_example.yaml

@@ -0,0 +1,54 @@
+# It is here as an example, and if the user wants to change the default


I approve of the thoroughness, but I'm not sure this is entirely necessary. Seems like a strange thing for the user to do.

mocks/mocks.py

AnthonyHorton · 2019-04-26T01:55:33Z

mocks/mocks.py

+    config = utils.load_yaml_config(config_file)
+    # Coordinates of the target galaxy.
+    config['galaxy_coordinates'] = kwargs.get('galaxy_coordinates',
+                                              config['galaxy_coordinates'])


Do you want default values for any of these parameters, in case they aren't in either the config file or kwargs? Can do this using the following pattern:

config[<parameter_name>] = kwargs.get(<parameter_name>, config.get(<parameter_name>, <default value>))

Might make sense to do this for parameters you don't expect to be changed by the user very often.

Should I choose any particular coordinate as the default or just something like what exists in the config file would work?

mocks/mocks.py

AnthonyHorton · 2019-04-26T02:40:36Z

mocks/mocks.py

+        kwargs.get('target_galaxy_comoving_depth',
+                   config['target_galaxy_comoving_depth'])
+    config['target_galaxy_comoving_depth'] = ensure_unit(
+        config['target_galaxy_comoving_depth'], u.Mpc)


This is all redundant now that it's being done in parse_config. You should remove it from here.

mocks/mocks.py

mocks/tests/test_mocks.py

bazkiaei · 2019-05-10T04:44:30Z

@AnthonyHorton
I tried to apply your advice. I had a meeting with @lspitler and he advised me to close this PR when parse_config is ready and start other new functions in other PRs. So if you agree with that and you are satisfied with this function I can start creating the other functions in other PRs, one by one.

lspitler

Some comments given. In retrospect, the discussion about cosmology and related tests might have been better suited for a separate PR as its separate to the way the config is loaded. But it seems it has merged here, so let's treat this PR as both finalising the expected cosmology and dealing with how to load the config.

mocks/mocks.py

lspitler · 2019-05-12T08:47:16Z

config_directory/config_no_cosmology_example.yaml

@@ -0,0 +1,43 @@
+# It is here as an example, and if the user wants to change the default


I don't think this is needed given my comments below.

lspitler · 2019-05-12T08:47:37Z

config_directory/config_example_old.yaml

@@ -0,0 +1,49 @@
+# It is here as an example, and if the user wants to change the default


I don't think this is needed given my comments below.

lspitler · 2019-05-12T08:48:52Z

config_directory/config_example.yaml

 # The resolution of the simulation(parsec / pixel).
-sim_pc_pixel: 170 
+sim_pc_pixel: 170

 # The baryonic mass of simulation particles (1e5 * Msol/h).
 particle_baryonic_mass_sim: 9.6031355

 # Cosmological parameters.


Different from WMAP9 for tests purposes

that sounds like a test of astropy.cosmology not huntsman-mocks, which shouldn't care about what cosmology is needed.

Are you sure you need this test?

mocks/mocks.py

bazkiaei · 2019-05-14T02:26:41Z

@lspitler
I tried to apply your comments. I used WMAP9 for the configuration file and canceled the default cosmology from parse_config. Ready to review.
@AnthonyHorton

lspitler

Looks good @bazkiaei I think this is ready after you address my remaining comments.

config_directory/config_example.yaml

mocks/tests/conftest.py

lspitler · 2019-05-14T02:56:52Z

mocks/mocks.py

@@ -23,6 +23,118 @@
 from mocks.utils import load_yaml_config


+def parse_config(config_file='config_example.yaml',


maybe this should move into utils?

I think we can use some advice from @AnthonyHorton and @wtgee about that.

@bazkiaei I like this definition:

Looks to me like /utils is a place where you can place small snippets you can use throughout the application. Small functions to build bigger things with.

from erikras/react-redux-universal-hot-example#808

You designed parse_config to be used specifically for mocks.py, so I guess it makes sense.

This is scope creep on the PR, so let's close this PR without changing this, but create a new Issue to decide where all mocks.py functions should go -- other ones I think meet the definition I copied in above. Or better yet add it to the discussion about creating a Class for Mocks: #63

mocks/mocks.py

lspitler · 2019-05-14T08:04:23Z

@AnthonyHorton your request for review is blocking this PR - can you Approve when you get a chance? I'm happy with @bazkiaei changes based upon your comments.

…t to the `config_example.yamle`.

AnthonyHorton · 2019-05-15T08:13:55Z

OK, looks good now. LGTM.

…d-config-outside

trying to load configuration outside of the functions

a196727

bazkiaei commented Apr 9, 2019

View reviewed changes

bazkiaei requested review from lspitler, AnthonyHorton and wtgee April 9, 2019 02:06

bazkiaei mentioned this pull request Apr 9, 2019

Loading the configuration outside of the functions. #67

Closed

docstring added

6ac350d

AnthonyHorton requested changes Apr 10, 2019

View reviewed changes

This was referenced Apr 12, 2019

Inconsistencies among the functions of mocks.py #70

Closed

Updating the jupyter-noteboods examples #71

Closed

bazkiaei added 2 commits April 12, 2019 14:33

improvement to tests applied

e8d3c1f

the scope of the fixture is changed

16c7896

bazkiaei commented Apr 12, 2019

View reviewed changes

mocks/tests/conftest.py Outdated Show resolved Hide resolved

fixture scope added

4ffc883

AnthonyHorton requested changes Apr 17, 2019

View reviewed changes

AnthonyHorton mentioned this pull request Apr 17, 2019

debugging the error with float32 data #72 #73

Merged

configuration file updated and inputs to scale_light_by_distance ar…

59f180d

…e changed

bazkiaei mentioned this pull request Apr 17, 2019

Dividing prepare_mocks and creating new functions. #74

Closed

AnthonyHorton reviewed Apr 17, 2019

View reviewed changes

parse_config added to the file

014d71b

bazkiaei commented Apr 18, 2019

View reviewed changes

bazkiaei commented Apr 19, 2019

View reviewed changes

AnthonyHorton requested changes Apr 26, 2019

View reviewed changes

corrections applied

2b5d442

lspitler requested changes May 12, 2019

View reviewed changes

removing conflicts

84f5afe

bazkiaei added 2 commits May 14, 2019 12:06

default cosmology canceled

6f56923

trying to fix coverage problem

c8a30bd

lspitler approved these changes May 14, 2019

View reviewed changes

bazkiaei mentioned this pull request May 14, 2019

Where all mocks.py functions should go? #78

Open

config_example_old.yaml deleted and the tests corrected with respec…

574efe0

…t to the `config_example.yamle`.

AnthonyHorton approved these changes May 15, 2019

View reviewed changes

Merge remote-tracking branch 'refs/remotes/upstream/develop' into loa…

3679f80

…d-config-outside

bazkiaei merged commit c903805 into AstroHuntsman:develop May 16, 2019

bazkiaei deleted the load-config-outside branch May 16, 2019 02:45

		@@ -21,12 +21,17 @@
		from mocks.utils import load_yaml_config


		def load_configuration(config_location='config_example.yaml'):

		@@ -0,0 +1,49 @@
		# It is here as an example, and if the user wants to change the default

		@@ -0,0 +1,43 @@
		# It is here as an example, and if the user wants to change the default

		@@ -0,0 +1,54 @@
		# It is here as an example, and if the user wants to change the default

		@@ -23,6 +23,118 @@
		from mocks.utils import load_yaml_config


		def parse_config(config_file='config_example.yaml',

Loading the configuration outside of the functions #67 #69

Loading the configuration outside of the functions #67 #69

Conversation

bazkiaei commented Apr 9, 2019

codecov bot commented Apr 9, 2019 • edited Loading

Codecov Report

bazkiaei Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

AnthonyHorton Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

bazkiaei Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

bazkiaei Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazkiaei Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

bazkiaei commented Apr 9, 2019

AnthonyHorton left a comment

Choose a reason for hiding this comment

AnthonyHorton Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazkiaei commented Apr 17, 2019

AnthonyHorton left a comment

Choose a reason for hiding this comment

AnthonyHorton Apr 17, 2019 • edited Loading

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

AnthonyHorton Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazkiaei left a comment • edited Loading

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

bazkiaei Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazkiaei Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazkiaei Apr 26, 2019 • edited Loading

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

bazkiaei Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazkiaei commented May 10, 2019

lspitler 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

bazkiaei commented May 14, 2019

lspitler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lspitler May 14, 2019 • edited Loading

Choose a reason for hiding this comment

lspitler commented May 14, 2019

AnthonyHorton commented May 15, 2019

codecov bot commented Apr 9, 2019 •

edited

Loading

bazkiaei Apr 9, 2019 •

edited

Loading

AnthonyHorton Apr 10, 2019 •

edited

Loading

bazkiaei Apr 12, 2019 •

edited

Loading

bazkiaei Apr 12, 2019 •

edited

Loading

bazkiaei Apr 12, 2019 •

edited

Loading

AnthonyHorton Apr 10, 2019 •

edited

Loading

AnthonyHorton Apr 17, 2019 •

edited

Loading

AnthonyHorton Apr 17, 2019 •

edited

Loading

bazkiaei left a comment •

edited

Loading

bazkiaei Apr 18, 2019 •

edited

Loading

bazkiaei Apr 19, 2019 •

edited

Loading

bazkiaei Apr 26, 2019 •

edited

Loading

bazkiaei Apr 26, 2019 •

edited

Loading

lspitler May 14, 2019 •

edited

Loading