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

Jump to bottom

Eliminate drivers and refactor configuration #259

Merged
merged 37 commits into from
Jun 28, 2017
Merged

Eliminate drivers and refactor configuration #259

merged 37 commits into from
Jun 28, 2017

Conversation

jacg
Copy link
Collaborator

@jacg jacg commented Jun 28, 2017 •
edited
Loading

  1. The driver functions for each city (the city name in uppercase) have been removed. They are all replaced with a single drive classmethod in City. This simplifies the code considerably and removes the copy-n-paste mess that has been growing an each new city was added.

  2. The city constructors no longer take and pass arguments explicitly by name, this is now achieved with **kwds. This closes Cities init method arguments #143 and has a number of consequences

    • Significant noise reduction in constructor definitions.
    • The config files become the sole source of
      • values of the parameters
      • documentation of what parameters are needed by each kind of city

  3. The config files are now written in Python syntax and parsed with Python's built-in parser. Our custom config parser for our ad-hoc config syntax has been thrown away and replaced with Python's parser. Do not re-invent the wheel.

  4. The ability to inculde config files within other config files has been added, allowing the use of hierarchical configurations. Thus the default configuration of some base city does not need to be repeated in each concrete city's config file, as in can be included.

  5. A new configuration printer has been added with the key abilities to

    • report the config file which provides any given value in an active configuration
    • highlight cases where a value set in one config file is overridden by a setting in another one.

    Improvements to the config printer are envisaged:

    • Displaying the comments from the config files.
    • Displaying the units used in the config files.

  6. The mandatory -c (config file) command line option has been turned into a positional argument.

(#203 has not been addressed at all here.)

jacg and others added 30 commits June 27, 2017 19:41
@jacg
Start removing drivers
3536da1 
The drivers are an unnecessary copy-n-past complication. They can be
replaced by a single classmethod which feeds the configuration
directly to the constructor of the relevant class. To make this work
smoothly, the constructor parameter names must match the config file
parameter names exactly.

While working on this change, we decided that we want a single source
of parameter values. At present, any parameter value might come from
the config file or from various constructor defaults. This will lead
to confusion and subtle errors. So we have started to eliminate the
default values in the constructors ...

... but then the units disappear, so we want units to be stated
explicitly in the config file, which requires the config file parser
to be rewritten to understand units.
@jacg
Parse config file with exec (Clean up Irene config and constructors)
d464e16 
The first step is to make it work for Irene. The other Cities will
follow.

The config file is now written in Python syntax and parsed with
`exec`. Like this we do not need write our own parsers, for our own
dodgy syntax with lots of dodgy exceptions. In short, DO NOT REINVENT
THE WHEEL.

Config file parsing is done in the context of `system_of_units`, this
allowing specification of units simply by multiplying the value by any
available unit, without qualification. Compound units are trivially
catered for in this scheme. It's just pure Python.

    my_value = 12 * nm / s

Consequently, everything disappears from the constructors:

+ parameter names
+ units
+ etc.

The config file is now the sole source of documentation of the city
parameters.

The driver functions (same name as city, but uppercase) now disappear:
a single `drive` method in the base city replaces them all.
@jacg
Clean up Diomira config and constructors
577ec80
@jacg
Clean up Isidora config and constructors
3aaf39b
@jacg
Clean up Dorothea config and constructors
eea4e41
@jacg
Clean up Zaira config and constructors
6f4ef33
@jacg
Clean up Cecilia config and constructors
7d36124
@jacg
Clean up Penthesilea config and constructors
60e14bb
@jacg
Adapt test_diomira_copy_mc_and_offset to new structure
559eefb
@jacg
Cosmetics: line length (soft) limit is now 100
309dd65
@jacg
Adapt test_dorothea_KrMC to new structure
73945b7
@jacg
Adapt test_irene_electrons_40keV to new structure
d8b589e
@jacg
Adapt test_irene_run_2983 friends to new structure
4fa19d4
@jacg
Adapt test_empty_events_issue_81 to new structure
1e5b963
@jacg
Adapt test_irene_40keV...correctly_set to new structure
d0d5275
@jacg
Add coherent tests for missing in/out files
fcacac1
@jacg
Adapt test_zaira_KrMC to new structure
8db6a6e
@jacg
Remove test_parse_value
0dff9f9 
We are no longer using our own parser: we let Python do the job for
us.
@jacg
Adapt test_configure to new structure
ea14f5b
@jacg
Remove path_in and path_out config parameters
4dba585 
These were used in combination with files_in and files out to to
produce the final files_in and files_out settings used by the cities.

This gained us nothing besides unnecessary complication of
configuration parsing and handling.
@jacg
Factor pmap_city.conf out of irene.conf
98ad6cf
@jacg
Factor superclass configurations out of pmap_city.conf
bd21f8e
@jacg
Factor superclass configurations out of isidora.conf
61b8842
@jacg
Factor superclass configurations out of diomira.conf
43d3ac8
@jacg
Factor superclass configurations out of dorothea.conf
8ce949d
@jjgomezcadenas @jacg
Updating configuration files
7cf0256
@jjgomezcadenas @jacg
Add s12_selector.conf and fix conf files.
0597281 
s12_selector.conf is added as part of the configuration tree, in spite
that there is no s12_selector city. It turns out that selectors are a
different species than cities in sofar that they cannot be passed by
inheritance (this would require double inheritance, something we want
to avoid) but at the same time they can be re-used by several
cities. Thus, it make sense to define default parameters for each
selector we introduce.

Several configuration files have been fixed to overwrite parameters
such as input/output file name, which are city-specific (although a
default is inherited from city).
@jacg
Clean up configure.py
ad59d53
@jacg
Implement hierarchical config parser and tests
b94f2c0
@jacg
Fix and test config override file tracking
f259e6b
jacg added 7 commits June 27, 2017 19:41
@jacg
Reduce noise relating to S12Selector construction
266fbcd
@jacg
Move verbosity and skip setting to city.conf
8e0d312 
It seems that skip is not used anywhere. Can it be removed?

Verbosity isn't used either, but at least it's clear how it *might* be
used.
@jacg
New configuration printer
27c2d0f 
The `--do-not-run` option has been added to allow inspection of a
configuration without running the given city.

By default, when an option is set in more than one place (multiple
places in the same config file, or in an included file aswell as an
including file), the printer shows the overridden values (the ones
which were specified before the last one). This can be switched off
with the `--no-overrides` option.

By default, the names of the files in which any value was
set (incuding overridden ones) is shown. By default, just the basename
of the file (everything after the last `/`) is shown. Whether and how
these file names are displayed can be controlled with the options

+ `--no-files`   : do not show the names at all
+ `--full-files` : show fully qualified file names

By default, the configuration is printed when any city is run. This
can be suppressed with `--hide-config`.
@jacg
Cater for lists and tuples (etc.) in config display
08d3ab6
@jacg
Turn cecilia.conf tr_channels from strs into ints
e8eecbb
@jacg
Rename option --do-not-run -> --print-config-only
9045bbd
@jacg
Turn -c CLI option into positional arg
e8d1287 
It is a mandatory argument, and it is the *only* mandatory
argument. Thus it makes sense to make it the only positional argument
on the CLI.
jjgomezcadenas
jjgomezcadenas approved these changes Jun 28, 2017
View reviewed changes
Copy link
Collaborator

@jjgomezcadenas jjgomezcadenas left a comment

Choose a reason for hiding this comment

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

A major advance in the code. Good and well!

@jacg jacg merged commit e8d1287 into next-exp:master Jun 28, 2017
@jacg jacg deleted the eliminate-drivers branch June 28, 2017 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjgomezcadenas jjgomezcadenas jjgomezcadenas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cities init method arguments
2 participants
@jacg @jjgomezcadenas