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.

Sign up for GitHub

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

Loading the configuration outside of the functions #67 #69

Merged
merged 13 commits into from
May 16, 2019
Merged

Loading the configuration outside of the functions #67 #69

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a196727
trying to load configuration outside of the functions
bazkiaei Apr 9, 2019
6ac350d
docstring added
bazkiaei Apr 9, 2019
e8d3c1f
improvement to tests applied
bazkiaei Apr 12, 2019
16c7896
the scope of the fixture is changed
bazkiaei Apr 12, 2019
4ffc883
fixture scope added
bazkiaei Apr 17, 2019
59f180d
configuration file updated and inputs to `scale_light_by_distance` ar…
bazkiaei Apr 17, 2019
014d71b
`parse_config` added to the file
bazkiaei Apr 18, 2019
2b5d442
corrections applied
bazkiaei May 10, 2019
84f5afe
removing conflicts
bazkiaei May 13, 2019
6f56923
default cosmology canceled
bazkiaei May 14, 2019
c8a30bd
trying to fix coverage problem
bazkiaei May 14, 2019
574efe0
`config_example_old.yaml` deleted and the tests corrected with respec…
bazkiaei May 14, 2019
3679f80
Merge remote-tracking branch 'refs/remotes/upstream/develop' into loa…
bazkiaei May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions mocks/mocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,17 @@
from mocks.utils import load_yaml_config


def load_configuration(config_location='config_example.yaml'):
Copy link
Contributor Author

@bazkiaei bazkiaei Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@AnthonyHorton AnthonyHorton Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bazkiaei bazkiaei Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bazkiaei bazkiaei Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

configuration = load_yaml_config(config_location)
return configuration


def scale_light_by_distance(particle_positions,
particle_values,
config,
physical_distance=10 * u.Mpc,
bazkiaei marked this conversation as resolved.
Show resolved Hide resolved
target_galaxy_comoving_depth=1 * u.Mpc,
bazkiaei marked this conversation as resolved.
Show resolved Hide resolved
viewing_axis='z',
config_location='config_example.yaml'):
viewing_axis='z'):
"""
Projecting 3D data to 2D data with respect to the depth of particles.

Expand Down Expand Up @@ -82,7 +87,6 @@ def scale_light_by_distance(particle_positions,
# Computing the length value that should be added to the positions of
# particles in the viewing axis (projection axis) as correction.
physical_distance = ensure_unit(physical_distance, u.Mpc)
config = load_yaml_config(config_location)
cosmo = create_cosmology(config)
comoving_correction_length = \
compute_correction_length(physical_distance,
Expand All @@ -108,7 +112,7 @@ 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,
Copy link
Member

@AnthonyHorton AnthonyHorton Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

**kwargs):
"""
Creates a dictionary containing configuration data and a numpy.array of
Expand All @@ -126,8 +130,6 @@ def prepare_mocks(config_location='config_example.yaml',
galaxy_sim_data_raw : numpy.array
Prepared information for creating mock images and the simulation data.
"""
# Loading configuration file:
config = load_yaml_config(config_location)
config['galaxy_coordinates'] =\
kwargs.get('galaxy_coordinates',
config.get('galaxy_coordinates',
bazkiaei marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
24 changes: 24 additions & 0 deletions mocks/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,27 @@ def custom_cosmology_data():
custom_data['hubble_constant'] = 70.
custom_data['T_CMB0'] = 2.5
return custom_data


@pytest.fixture
bazkiaei marked this conversation as resolved.
Show resolved Hide resolved
def configuration():
config = dict()
config['galaxy_coordinates'] = '14h40m56.435s -60d53m48.3s'
config['observation_time'] = '2018-04-12T08:00'
config['data_path'] = 'sim_data/cl19.fits'
config['imager_filter'] = 'g'
config['mass_to_light_ratio'] = dict()
config['mass_to_light_ratio']['g'] = 5
config['mass_to_light_ratio']['r'] = 5
config['abs_mag_sun'] = dict()
config['abs_mag_sun']['g'] = 5.11
config['abs_mag_sun']['r'] = 4.68
config['abs_mag_sun']['i'] = 4.53
config['galaxy_distance'] = 10.
config['sim_pc_pixel'] = 170
config['particle_baryonic_mass_sim'] = 9.6031355
config['hubble_constant'] = 0.705
config['Omega_Lambda'] = 0.727
config['Omega_0'] = 0.273
config['redshift'] = 0
return config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bazkiaei bazkiaei Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

20 changes: 15 additions & 5 deletions mocks/tests/test_mocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,15 @@ def pixelated_psf_data(huntsman_sbig_dark_imager):
return psf_data


def test_prepare_mocks():
mocks_config, data = mocks.prepare_mocks()
def test_load_configuration():
config = mocks.load_configuration()
assert config['data_path'] == 'sim_data/cl19.fits'
assert config['galaxy_distance'] == 10.
assert type(config) == dict
bazkiaei marked this conversation as resolved.
Show resolved Hide resolved


def test_prepare_mocks(configuration):
mocks_config, data = mocks.prepare_mocks(configuration)
assert mocks_config['galaxy_coordinates'] == '14h40m56.435s -60d53m48.3s'
assert mocks_config['observation_time'] == '2018-04-12T08:00'
assert mocks_config['imager_filter'] == 'g'
Expand All @@ -47,8 +54,9 @@ def test_prepare_mocks():
rel=1e-12)


def test_create_mock_galaxy_noiseless_image(huntsman_sbig_dark_imager):
config, galaxy_sim_data = mocks.prepare_mocks()
def test_create_mock_galaxy_noiseless_image(huntsman_sbig_dark_imager,
configuration):
config, galaxy_sim_data = mocks.prepare_mocks(configuration)
noiseless_image = \
mocks.create_mock_galaxy_noiseless_image(config,
galaxy_sim_data,
Expand All @@ -73,10 +81,12 @@ def test_create_mock_galaxy_noiseless_image(huntsman_sbig_dark_imager):


def test_scale_light_by_distance(particle_positions_3D,
mass_weights):
mass_weights,
configuration):
positions, luminosity =\
mocks.scale_light_by_distance(particle_positions_3D,
mass_weights,
configuration,
10,
4 * u.Mpc)
assert type(positions) == np.ndarray
Expand Down