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.

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

Global parameter #206

Merged
merged 2 commits into from Sep 25, 2017

Conversation

Projects
None yet
2 participants
@benjello
Copy link
Collaborator

commented Jan 13, 2016

For now I am able to introduce a DEBUG parameters.
@gdementen could you tell me if this seems ok and if I should proceed to the inclusion of any parameters set to a non iterable variable (or a variable that have with neither path nor defined in the node globals of the hdf file).

@@ -40,7 +40,12 @@ def global_symbols(globals_def):
# entity. I guess they should have a class of their own
symbols = {}
for name, global_def in globals_def.iteritems():
global_type = global_def.get('fields')
try:

This comment has been minimized.

Copy link
@gdementen

gdementen Jan 29, 2016

Member

I'd prefer an isinstance check in this case

@@ -100,6 +100,7 @@ class Simulation(object):
yaml_layout = {
'import': None,
'globals': {
'DEBUG': None,

This comment has been minimized.

Copy link
@gdementen

gdementen Jan 29, 2016

Member

please revert this line. A debug flag usable in user code would make sense, but having it in globals seems weird. Globals (in my opinion) are for user/model-defined stuff, while debug should be set by the system (e.g. the command-line argument).

EDIT: Oups, I thought this was in the globals "storage". The line should still be reverted, but for a different reason than what I say above... The validator code needs to be updated to allow for a scalar value OR a dict. Hardcoding DEBUG is not a solution, as you cannot define any other "scalar"/inline globals, as I thought this PR was all about...

@gdementen

This comment has been minimized.

Copy link
Member

commented Jan 29, 2016

Ok, so this PR looks good in principle. The implementation is a bit ugly to my taste (no real objective criticism), but I can fix that myself, but that validator thing is a no-go. Might need a separate PR for that. I would personally write that as a class:

class Or(object):
    def __init__(self, choices):
        ....

# and in the layout, you would use:
yaml_layout = {
         'import': None,
         'globals': {
          '*': Or(str, int, float, bool, {
                 'path': str,
                 'type': str,
                 'fields': [{
                     '*': None  # Or(str, {'type': str, 'initialdata': bool})
                 }],
                 'oldnames': {
                     '*': str
                 },
                 'newnames': {
                     '*': str
                 },
                 'invert': [str],
                 'transposed': bool
             })

or something like that...

@gdementen gdementen modified the milestone: 0.12 Apr 19, 2016

@benjello benjello force-pushed the benjello:global_parameter branch from 6db15eb to 8452875 Apr 19, 2017

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 19, 2017

@gdementen : I made some changes following your remarks. Feel free to ask more changes.

globals_node = getattr(input_root, 'globals', None)
for name, global_def in globals_def.iteritems():
# already loaded from another source (path)
if name in globals_data:
continue

if name not in globals_node:
if isinstance(global_def, numbers.Number) or isinstance(global_def, bool):

This comment has been minimized.

Copy link
@gdementen

gdementen Apr 19, 2017

Member

wouldn't if np.isscalar(global_def) be more robust? I would move the check above, so that None of the code below is executed (it seems useless in this case). In fact, I would just add these lines just below globals_data = load_path_globals(globals_def):

globals_data.update({k: v for k, v in globals_def.iteritems()
                     if np.isscalar(v)})

EDIT: In fact, I think this code is completely useless now that the constants are handled before the loop (currently in load_path_globals, but if you handle them in handle_constant_globals, that would be the same.

# provided in the file)
v["fields"] = fields_yaml_to_type(v["fields"])
globals_def[k] = v
except TypeError:

This comment has been minimized.

Copy link
@gdementen

gdementen Apr 19, 2017

Member

I would rather use an np.isscalar(v) check than a try except

@@ -710,12 +711,15 @@ def load_path_globals(globals_def):
localdir = config.input_directory
globals_data = {}
for name, global_def in globals_def.iteritems():
if 'path' not in global_def:
if 'path' not in global_def and 'value' not in global_def:

This comment has been minimized.

Copy link
@gdementen

gdementen Apr 19, 2017

Member

"constant" globals shouldn't be handled in load_path_globals. Either in index_tables directly or in a separate method "handle_constant_globals".

@@ -853,6 +858,11 @@ def prepare(self, globals_def, entities, input_dataset, start_period):
output_globals = output_file.create_group("/", "globals",
"Globals")
for k, g_def in globals_def.iteritems():
# Do not save global constants ie those who have a value
# attribute
if 'value' in g_def:

This comment has been minimized.

Copy link
@gdementen

gdementen Apr 19, 2017

Member

maybe change the test to if 'value' in g_def or 'path' in g_def: so that the if 'path' not in g_def test can be removed?

@@ -562,6 +562,9 @@ def load_def(localdir, ent_name, section_def, required_fields):
"type and fields sections are mutually exclusive"
% ent_name)

if "value" in section_def:
return 'scalar', section_def["value"]

This comment has been minimized.

Copy link
@gdementen

gdementen Apr 19, 2017

Member

remove this change

@@ -1081,6 +1082,8 @@ def validate_value(v, target, context):
validate_dict(v, target, context)
elif isinstance(target, list):
validate_list(v, target, context)
elif isinstance(target, numbers.Number) or isinstance(target, bool):

This comment has been minimized.

Copy link
@gdementen

gdementen Apr 19, 2017

Member

this shouldn't be needed since you are using None as validator for the 'value' keyword

@gdementen
Copy link
Member

left a comment

Please also:

  • revert your changes to demo.h5 and small.h5 (unless you have a good reason, but then it needs to be documented)
  • add a changelog line
  • add the corresponding documentation
@@ -853,8 +869,12 @@ def prepare(self, globals_def, entities, input_dataset, start_period):
output_globals = output_file.create_group("/", "globals",
"Globals")
for k, g_def in globals_def.iteritems():
if 'path' not in g_def:
anyarray_to_disk(output_globals, k, globals_data[k])
# Do not save global constants ie those who have a value

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member

Do not save global constants (ie those who have a value attribute) nor globals loaded from external (.csv) files.

@@ -2796,6 +2808,11 @@ entities:
fields=[('PERIOD', int), ('INTFIELD', int), ('FLOATFIELD', float)])
- assertEqual(table.shape, (31,))

test_global_parameter:
- assertTrue(BOOL_CONSTANT)
- assertTrue(FLOAT_CONSTANT == 3.1415)

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member

use assertEqual instead?

def handle_constant_globals(globals_def):
globals_data = {}
for name, global_def in globals_def.iteritems():
if 'value' not in global_def:

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member
  • unsure restricting to scalars adds any value. Using constant array globals does not seem like such a bad idea.
  • the "or isinstance(value, bool)" part is useless (AFAICT) since np.isscalar(True) is True and np.isscalar(False) is True
    So I would rewrite the content of the loop to something like:
if 'value' in global_def:
    globals_data[name] = global_def['value']
@@ -41,7 +41,12 @@ def global_symbols(globals_def):
# entity. I guess they should have a class of their own
symbols = {}
for name, global_def in globals_def.iteritems():
global_type = global_def.get('fields')
if isinstance(global_def, dict):

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member

this change is useless AFAICT: it would be useful if you could write:

BOOL_CONSTANT: True
FLOAT_CONSTANT: 3.1415
INT_CONSTANT: 42

but the current "yaml validator" forbids that.

EDIT: I have just seen this is useful after all (because of the code in from_str) but I would rather have that changed. and Ideally, the case where value is given, type should be optional (ie inferred using type(value))

@@ -241,6 +242,12 @@ def from_str(cls, yaml_str, simulation_dir='',
# 'MIG': {'type': int}}
globals_def = {}
for k, v in content.get('globals', {}).iteritems():
if 'value' in v:
if np.isscalar(v['value']) or isinstance(v['value'], bool):

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member
  • unsure restricting to scalars adds any value
  • isinstance(v['value'], bool) seems useless
@@ -241,6 +242,12 @@ def from_str(cls, yaml_str, simulation_dir='',
# 'MIG': {'type': int}}
globals_def = {}
for k, v in content.get('globals', {}).iteritems():
if 'value' in v:

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member

this whole block of change could be removed IMO because it is mostly unneeded. All it does is transform {'INT_CONSTANT': {'value': 42, 'type': 'int'}} to {'INT_CONSTANT': 42}

I would rather avoid doing that and simply handle the "value" case in globals_symbols. Besides, I prefer if from_str is does not change the structure of the globals definition, just convert string (types) to real types.

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 22, 2017

Modifications and rebasing done.

kind, info = load_def(localdir, name, global_def, [])
if kind == 'table':
fields, numlines, datastream, csvfile = info
array = stream_to_array(fields, datastream, numlines)
elif kind == 'scalar':

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member

are these two lines of any use? I do not see anything producing that kind = "scalar"???

@@ -43,6 +43,21 @@
* implemented experimental align(link=) to use proportions in combination with the Chenard algorithm. It needs more
testing though.

* implemented a way of declaring global constants. These constant should be explicitely typed. (partially closes :issue:`203`): ::

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 22, 2017

Member

why partially?

This comment has been minimized.

Copy link
@benjello

benjello Sep 23, 2017

Author Collaborator

We cannot use:

    INT_CONSTANT: 42

This comment has been minimized.

Copy link
@gdementen

gdementen Sep 25, 2017

Member

I consider the feature implemented, even though the syntax is not ideal. We can still improve the syntax later, but I will close the issue anyway.

@benjello benjello force-pushed the benjello:global_parameter branch 2 times, most recently from 3888b22 to e2f82b7 Sep 24, 2017

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 24, 2017

Rebase done

@benjello benjello force-pushed the benjello:global_parameter branch from e2f82b7 to 88542c0 Sep 24, 2017

@gdementen
Copy link
Member

left a comment

LGTM. Thanks!

@gdementen gdementen merged commit 08f6f0c into liam2:master Sep 25, 2017

2 checks passed

code-quality/landscape Code quality increased by 0.02%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjello benjello deleted the benjello:global_parameter branch Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.