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

Fix py3 issues #620

Merged
merged 6 commits into from Jan 6, 2017
Merged

Fix py3 issues #620

merged 6 commits into from Jan 6, 2017

Conversation

@heplesser
Copy link
Contributor

heplesser commented Jan 5, 2017

This PR fixes a number of Python 3 issues, including incorrect imports in pynest/nest/lib, thus addressing #617 among other issues. It is a superset of #618.

Copy link
Member

Silmathoron left a comment

Ok, mostly good for me, I'm unsure about making SuppressDeprecationWarning available to users, though...

print 'Number of populations:'
print num_pops
print
print('Number of populations:')

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

Wouldn't print('Number of populations: {}\n'.format(num_pops)) look nicer?

This comment has been minimized.

Copy link
@heplesser

heplesser Jan 5, 2017

Author Contributor

I agree with you in principle, but in this case I prioritised getting things done over doing them perfectly. Example beautification can come in a separate PR later.

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

No problem

print 'Population sizes:'
print Pop_sizes
print
print('Population sizes:')

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

See above

print 'Total number of neurons:'
print num_neurons
print
print('Total number of neurons:')

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

See above

print
print 'Firing rates:'
print rates
print()

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

see above

@@ -59,10 +59,10 @@ def write_help_html(doc_dic, helpdir, fname, sli_command_list, keywords):
hlplist = []
name = ''

for key, value in doc_dic.iteritems():
for key, value in doc_dic.items():

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

I don't know how much there is to do, but iter(doc_dic.items()) would be better to keep the spirit of the previous code

This comment has been minimized.

Copy link
@heplesser

heplesser Jan 5, 2017

Author Contributor

I think wrapping with iter() will not provide any gains and rather make the code less readable. The only point of using iteritems() in Python 2 is to get a generator instead of a full list of items. iter(d.items()) will still create the full list and then wrap an iterator around it in Python 2. And for k, v in d.items(): is the pythonic way as far as I know.

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

Ok, I thought they would find a clever way to do this in py2... but if the list is created, then there is indeed really no point


# Need to reset the deprecation warning to its old value
hlh._deprecation_warning['GetChildren'] = deprecation_bool
# restore_deprecation_warning('GetChildren', deprecation_status)

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

why is this here if it is commented (same question for L457)

This comment has been minimized.

Copy link
@heplesser

heplesser Jan 5, 2017

Author Contributor

Fixed.

def turn_off_deprecation_warning(deprecated_func_name):
"""Turns off deprecation warnings on SLI and Python level for given
function.
class SuppressedDeprecationWarning(object):

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

I think this should not be exposed to users, so maybe add a _ before and a __all__ in the file?

This comment has been minimized.

Copy link
@heplesser

heplesser Jan 5, 2017

Author Contributor

Fixed. I will add explicit imports where needed.

This comment has been minimized.

Copy link
@heplesser

heplesser Jan 5, 2017

Author Contributor

I thought about it a little more. SuppressedDeprecationWarning is also needed in the Topology hl_api, so I would have to access it there as a private member; possible, but slightly breaking with the logic of privacy. It is also not an implementation detail that may change at any time and does not require privacy from that perspective. Users should realise that they should use this context with care based on its name and documentation. Thus, I will leave the name as it is after all.

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

Fair enough

@@ -402,6 +400,38 @@ def broadcast(item, length, allowed_types, name="item"):
return item


@check_stack

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

Why did you switch it here? (I don't mind, though, just curious)

This comment has been minimized.

Copy link
@heplesser

heplesser Jan 5, 2017

Author Contributor

I ran into a circular inclusion problem otherwise. The SuppressedDeprecationWarning class definition fits best here among the helpers. It needs to manipulate the verbosity level. Having the verbosity functions defined in hl_api_info.py requires helper to include info an vice-versa. Moving the verbosity function definitions here means that hl_api_helper.py does not need to include any other hl_api_*.py files.

This comment has been minimized.

Copy link
@Silmathoron
verbosity_level)

# Avoid confusing user by deprecation warning
with SuppressedDeprecationWarning('CurrentSubnet'):

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

Note that if you indeed chose to make this not available to users (see comment above), you'll need to import it explicitly

# Need to reset the deprecation warning to its old value
nest.turn_on_deprecation_warning('GetLeaves', deprecation_bool,
verbosity_level)
with nest.SuppressDeprecationWarning('GetLeaves'):

This comment has been minimized.

Copy link
@Silmathoron

Silmathoron Jan 5, 2017

Member

Same comment as above on accessibility of this function

@heplesser
Copy link
Contributor Author

heplesser commented Jan 5, 2017

@Silmathoron Thank you for your fast review! I just pushed some changes which should also fix the PEP8 problem that Travis choked on.

Copy link
Member

Silmathoron left a comment

All good! 👍

@heplesser heplesser merged commit e58150e into nest:master Jan 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
heplesser added a commit to heplesser/nest-simulator that referenced this pull request Jan 6, 2017
Copy link
Collaborator

steffengraber left a comment

:+1 Too late, but it looks fine

@heplesser heplesser deleted the heplesser:fix-py3-issues branch Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.