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

Tightened semantics of Dimension objects #1245

Merged
merged 41 commits into from Apr 8, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Member

jlstevens commented Apr 6, 2017

This PR aims to address issue #843 and clean-up the definition of Dimension objects generally.

It is still work in progress and I've started this PR at the first commit just so that I can see the results of the notebook tests as I progress.

Edit: I have copied over the action items from the issue:

  • Renaming the first argument from name to spec.
  • Remove the deprecated 'initial' option from the values parameter.
  • Declaring label as a parameter
  • Allowing label to be passed as a kwarg and warning when ambiguous.
  • Fixing the __call__ method, making it work with (name, label) tuples
  • Aliased __call__ to clone.
  • Implemented the spec property
  • Update class docstring
  • Updated equality semantics
  • Updated comparison semantics
  • Add more unit tests.

@jlstevens jlstevens added the WIP label Apr 6, 2017

raise Exception("Values argument can only be set with the string 'initial'.")
values = params.get('values', [])
if isinstance(values, basestring) and values == 'initial':
self.warning("The 'initial' string for dimension values is no longer supported.")

This comment has been minimized.

@philippjfr

philippjfr Apr 6, 2017

Member

Unless I'm confused set values to an empty list here, otherwise param will throw an exception after you've already warned.

This comment has been minimized.

@philippjfr

philippjfr Apr 6, 2017

Member

Actually no, you'll get ['a', 'i', 'i', 'i', 'l', 'n', 't'].

This comment has been minimized.

@jlstevens

jlstevens Apr 6, 2017

Member

Fixed in 9d57a58 (I pushed forced an amended commit)

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 6, 2017

Looks good to me and I'm happy with the pprint.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 7, 2017

Great!

I'm happy the tests have passed - I was worried there might be double specification of name somewhere. I'll now update the equality semantics and see if the tests still pass...

I am also considering making __str__ return only self.name so equality with strings makes sense. This means using pprint_label - which I would also consider aliasing to a nicer name, pretty, so it would be dim.pretty.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 7, 2017

Rebasing against current master to get the push builds passing (pr builds are green).

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 7, 2017

The pr build has now passed for Python 3 and 3. I will now try to eliminate old uses of str(dim) and make sure pprint_label is used instead.

Note, the next commit is one that I'll use to find where dimensions are cast to strings. It uses some nasty inspection code and I'll keep amending and push forcing that commit till I've caught them all.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 7, 2017

Rebasing again due to recent notebook test data updates.

@@ -64,7 +63,8 @@ def replace_dimensions(dimensions, overrides):
elif isinstance(override, Dimension):
replaced.append(override)
elif isinstance(override, dict):
replaced.append(d(**override))
replaced.append(d(override.get('name',None),

This comment has been minimized.

@philippjfr

philippjfr Apr 7, 2017

Member

Should be using .clone internally now?

This comment has been minimized.

@jlstevens

jlstevens Apr 7, 2017

Member

Yes, __call__ is now an alias to clone but I do want to recommend clone from now on. Thanks for point it out and I'll fix it shortly.

This comment has been minimized.

@jlstevens
elif (name,) in self.presets.keys():
existing_params = dict(self.presets[(str(name),)].get_param_values())
elif spec in self.presets.keys():
existing_params = dict(self.presets[str(spec)].get_param_values())

This comment has been minimized.

@philippjfr

philippjfr Apr 7, 2017

Member

This doesn't seem right, you check spec is in presets but then do the lookup with str(spec)? Same below, also is .keys() needed in the conditional?

This comment has been minimized.

@jlstevens

jlstevens Apr 7, 2017

Member

Addressed in 703c01d

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 7, 2017

Tests have passed and I think this PR is now ready to review.

if name is not None: settings['name'] = name
return self.__class__(**settings)
spec = spec if (spec is not None) else (overrides.get('name', self.name),
overrides.get('label', self.label))

This comment has been minimized.

@philippjfr

philippjfr Apr 7, 2017

Member

Don't want to check if name or label are supplied along with a spec?

This comment has been minimized.

@jlstevens

jlstevens Apr 7, 2017

Member

Does it matter? If there is a spec, it gets passed through as the first argument, shouldn't matter if it is just a string or just a tuple.

This comment has been minimized.

@philippjfr

philippjfr Apr 7, 2017

Member

Hmm, since you're filtering out name and label from kwargs below, those will simply be silently be dropped.

This comment has been minimized.

@jlstevens

jlstevens Apr 7, 2017

Member

Ah got you. Fixed in 6d50a8a.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 7, 2017

@philippjfr I've addressed your comments and the tests are passing. Any other suggestions for dimension improvements?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 8, 2017

No, should I merge?

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 8, 2017

If you are happy with the changes, please do!

@philippjfr philippjfr merged commit 4d077dc into master Apr 8, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 78.609%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens referenced this pull request Apr 9, 2017

Closed

Dynamic Map step size #742

@philippjfr philippjfr deleted the dimension_cleanup branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment