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

Deprecation warnings for features that will disappear (fixes #552) #580

Merged
merged 18 commits into from Jan 4, 2017

Conversation

Projects
None yet
3 participants
@stinebuu
Contributor

stinebuu commented Dec 7, 2016

Added deprecation warnings for features that will disappear in NEST 3, which includes all subnet functions, the subnet model, DataConnect() and the aeif_cond_alpha_RK5 model. This fixes #552.

The PR also includes partial work on #558, by making sure the examples do not display introduced deprecation warnings.

I suggest @heplesser and @jougs as reviewers.

@heplesser

@stinebuu Fine overall, but I have some suggestions on details.

Show outdated Hide outdated pynest/nest/lib/hl_api_models.py
@@ -186,6 +186,16 @@ def CopyModel(existing, new, params=None):
Default parameters assigned to the copy. Not provided parameters are
taken from the existing model.
"""
try:
deprecated_models = ['subnet', 'aeif_cond_alpha_RK5']

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

I would define a list of deprecated models at the module level. In that way, you can use the same list in all places that need it, avoiding code duplications. This will make things easier if we change the list of deprecated models in later versions. See also comment on Create() below, may turn this entire try into helper function.

Instead of a list, you could also turn this into a dictionary, mapping the name of the deprecated model to a text for what to do otherwise, e.g., suggest used of aeif_cond_alpha.

Why do you need the try-except here? Are there models without type_id? If that is the problem, could you make the try part more local?

@heplesser

heplesser Dec 8, 2016

Contributor

I would define a list of deprecated models at the module level. In that way, you can use the same list in all places that need it, avoiding code duplications. This will make things easier if we change the list of deprecated models in later versions. See also comment on Create() below, may turn this entire try into helper function.

Instead of a list, you could also turn this into a dictionary, mapping the name of the deprecated model to a text for what to do otherwise, e.g., suggest used of aeif_cond_alpha.

Why do you need the try-except here? Are there models without type_id? If that is the problem, could you make the try part more local?

Show outdated Hide outdated pynest/nest/lib/hl_api_nodes.py
text = get_wrapped_text(text)
warnings.warn('\n' + text)
except(KeyError, NESTError):
pass

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

This is almost the same code as in CopyModels() above. Maybe turn it into a helper function you can call from both places? And why do you also catch NESTError here, but not above?

@heplesser

heplesser Dec 8, 2016

Contributor

This is almost the same code as in CopyModels() above. Maybe turn it into a helper function you can call from both places? And why do you also catch NESTError here, but not above?

Show outdated Hide outdated pynest/nest/lib/hl_api_nodes.py
@@ -72,6 +85,8 @@ def Create(model, n=1, params=None):
@check_stack
@deprecated("", "GetLID: LIDs disappear with subnets, use index into "
"GIDCollection")

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

Maybe better: "GetLID will be removed in NEST3. Use ..."

@heplesser

heplesser Dec 8, 2016

Contributor

Maybe better: "GetLID will be removed in NEST3. Use ..."

Show outdated Hide outdated topology/pynest/hl_api.py
@@ -1610,6 +1611,9 @@ def GetTargetNodes(sources, tgt_layer, tgt_model=None, syn_model=None):
if len(tgt_layer) != 1:
raise nest.NESTError("tgt_layer must be a one-element list")
# Turn of deprecation warning as users shouldn't change implementation of
# GetTargetNodes, it is done by the developers
hlh._deprecation_warning['GetLeaves'] = False

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

After GetLeaves() is done, you need to reset hlh._deprecation_warning to its old value. Applies also to the other functions below.

@heplesser

heplesser Dec 8, 2016

Contributor

After GetLeaves() is done, you need to reset hlh._deprecation_warning to its old value. Applies also to the other functions below.

Show outdated Hide outdated topology/examples/ctx_2n.py
nest.PrintNetwork(2, ctx)
gids = range(ctx[0] + 1, col*row*len(el) + ctx[0] + 1)

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

I wonder if the cure here isn't worse than the problem ... The examples should show users how to do things, but we now will just confuse them with a work-around for a deprecation warning. A better way, I think would be to write these examples like this:

# Create Layer. ctx_subnet is a work-around until NEST3.
ctx_subnet = topo.CreateLayer(...)
ctx = nest.GetLeaves(ctx_subnet)[0]   
...
# use ctx in remaining example code

In this way, we isolate the problem in one place with a proper comment.

@heplesser

heplesser Dec 8, 2016

Contributor

I wonder if the cure here isn't worse than the problem ... The examples should show users how to do things, but we now will just confuse them with a work-around for a deprecation warning. A better way, I think would be to write these examples like this:

# Create Layer. ctx_subnet is a work-around until NEST3.
ctx_subnet = topo.CreateLayer(...)
ctx = nest.GetLeaves(ctx_subnet)[0]   
...
# use ctx in remaining example code

In this way, we isolate the problem in one place with a proper comment.

Show outdated Hide outdated topology/examples/ctx_2n.py
nest.PrintNetwork(2)
nest.PrintNetwork(2, (0,))
nest.PrintNetwork(2, ctx)

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

I think we should use the opportunity here to improve the example. All these different PrintNetwork calls make very limited sense, and PrintNetwork will change anyways in NEST3. So I would reduce it to just one call to

nest.PrintNetwork(2)

I we then suppress the deprecation warning for CurrentSubnet() in PrintNetwork(), all should work nicely.

@heplesser

heplesser Dec 8, 2016

Contributor

I think we should use the opportunity here to improve the example. All these different PrintNetwork calls make very limited sense, and PrintNetwork will change anyways in NEST3. So I would reduce it to just one call to

nest.PrintNetwork(2)

I we then suppress the deprecation warning for CurrentSubnet() in PrintNetwork(), all should work nicely.

Show outdated Hide outdated pynest/nest/lib/hl_api_subnets.py
@@ -277,6 +287,8 @@ def EndSubnet():
@check_stack
@deprecated("", "LayoutNetwork: use Create(<model>, n=<number>) and get a "
"GIDCollection")

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

Better: "LayoutNetwork is deprecated and will be removed in NEST3. Use Create(, n=) instead."

@deprecated('LayoutNetwork', 'Create(<model>, n=<number>)')
@heplesser

heplesser Dec 8, 2016

Contributor

Better: "LayoutNetwork is deprecated and will be removed in NEST3. Use Create(, n=) instead."

@deprecated('LayoutNetwork', 'Create(<model>, n=<number>)')
Show outdated Hide outdated pynest/nest/lib/hl_api_subnets.py
@@ -236,6 +244,7 @@ def GetNetwork(gid, depth):
@check_stack
@deprecated("", "BeginSubnet: no longer needed, work with GIDCollections")

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

Better: "BeginSubnet is deprecated and will be removed in NEST3. See documentation for details."

@heplesser

heplesser Dec 8, 2016

Contributor

Better: "BeginSubnet is deprecated and will be removed in NEST3. See documentation for details."

Show outdated Hide outdated pynest/nest/lib/hl_api_connections.py
@@ -350,6 +350,7 @@ def Connect(pre, post, conn_spec=None, syn_spec=None, model=None):
@check_stack
@deprecated("", "DataConnect: use Connect() with one_to_one rule")

This comment has been minimized.

@heplesser

heplesser Dec 8, 2016

Contributor

Better: @deprecated('DataConnect', 'Connect() with one_to_one rule'). In that way, the message will automatically have standard format.

@heplesser

heplesser Dec 8, 2016

Contributor

Better: @deprecated('DataConnect', 'Connect() with one_to_one rule'). In that way, the message will automatically have standard format.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Dec 8, 2016

Contributor

I have a very general remark: shouldn't deprecation warnings be handled on the SLI level wherever possible? This way they'd automatically bubble up to PyNEST and would only have to be implemented once.

For models, we could for example add two flags in the Node base class (deprecated, deprecation_warning_issued) that are checked in all functions dealing with nodes. A similar mechanism could be thought of for SLIFunctions, where the interpreter could issue warnings whenever a function is used.

Contributor

jougs commented Dec 8, 2016

I have a very general remark: shouldn't deprecation warnings be handled on the SLI level wherever possible? This way they'd automatically bubble up to PyNEST and would only have to be implemented once.

For models, we could for example add two flags in the Node base class (deprecated, deprecation_warning_issued) that are checked in all functions dealing with nodes. A similar mechanism could be thought of for SLIFunctions, where the interpreter could issue warnings whenever a function is used.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 8, 2016

Contributor

@jougs Since we already had the nice @deprecated decorator in PyNEST and almost all end users use PyNEST today, I figured that doing it at the Python level would be easier and cover most cases.

Contributor

heplesser commented Dec 8, 2016

@jougs Since we already had the nice @deprecated decorator in PyNEST and almost all end users use PyNEST today, I figured that doing it at the Python level would be easier and cover most cases.

stinebuu added some commits Dec 9, 2016

Changed the deprecation warnings, made a helper function for models that
are deprecated, made sure PrintNetwork does not display deprecation
warning, re-did examples.
@stinebuu

This comment has been minimized.

Show comment
Hide comment
@stinebuu

stinebuu Dec 9, 2016

Contributor

@heplesser I have made changes in accordance to your comments if you want to take a look.

Contributor

stinebuu commented Dec 9, 2016

@heplesser I have made changes in accordance to your comments if you want to take a look.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 12, 2016

Contributor

@jougs We have now added support for deprecation at the C++ level. I am wondering what to do about the SLI examples. We would have to write quite a bit of code programming around the lack of full support for GIDCollections, which will simplify significantly again in NEST 3. Therefore, I wonder if it isn't better to leave the SLI examples as they are, even though the user then will see some warnings. The problem does not (or to a much lesser degree) exist at the PyNEST level, since lists of GIDs there behave almost like GIDCollections will in NEST 3.

Contributor

heplesser commented Dec 12, 2016

@jougs We have now added support for deprecation at the C++ level. I am wondering what to do about the SLI examples. We would have to write quite a bit of code programming around the lack of full support for GIDCollections, which will simplify significantly again in NEST 3. Therefore, I wonder if it isn't better to leave the SLI examples as they are, even though the user then will see some warnings. The problem does not (or to a much lesser degree) exist at the PyNEST level, since lists of GIDs there behave almost like GIDCollections will in NEST 3.

stinebuu added some commits Dec 13, 2016

Updated sli deprecation, changed iaf_neuron to iaf_cond_alpha in PyNEST
examples, made sure deprecation warnings are issued when CreateLayer is
used.

stinebuu added some commits Dec 15, 2016

Added M_DEPRECATED to list of possible messages in order to suppress
some SLI-deprecations on Python level. Made sure SLI remember if
deprecation warnings were issued for GetNetwork and LayoutNetwork. Made
helper function to supress and reset deprecation warnings on Python
level.
@stinebuu

This comment has been minimized.

Show comment
Hide comment
@stinebuu

stinebuu Dec 15, 2016

Contributor

There are now deprecation warnings on PyNEST and SLI level. Some PyNEST examples, like ctx_2n.py and hill_tononi_Vp.py still show deprecation warnings, but I have made comments saying it's a work-around. I have not done anything with the SLI-examples.

Contributor

stinebuu commented Dec 15, 2016

There are now deprecation warnings on PyNEST and SLI level. Some PyNEST examples, like ctx_2n.py and hill_tononi_Vp.py still show deprecation warnings, but I have made comments saying it's a work-around. I have not done anything with the SLI-examples.

@heplesser

Looks quite good, just a few language fixes needed.

Show outdated Hide outdated topology/examples/test_3d_exp.py
@@ -27,6 +27,10 @@
3d layers are currently not supported, use at your own risk!
Hans Ekkehard Plesser, UMB
This example uses the function GetChildren, which is deprecated. A deprecation
warning is therefore released. For details about deprecated functions, see

This comment has been minimized.

@heplesser

heplesser Dec 16, 2016

Contributor

released -> issued (applies also in other places)

@heplesser

heplesser Dec 16, 2016

Contributor

released -> issued (applies also in other places)

Show outdated Hide outdated pynest/nest/lib/hl_api_helper.py
if model in deprecated_models:
text = "The {0} model is deprecated and will be removed in a \
future verstion of NEST, use {1} instead\
".format(model, deprecated_models[model])
text = get_wrapped_text(text)
warnings.warn('\n' + text)
def turn_of_deprecation_warning(deprecated_func_name):

This comment has been minimized.

@heplesser

heplesser Dec 16, 2016

Contributor

turn_of_... -> turn_off_...

@heplesser

heplesser Dec 16, 2016

Contributor

turn_of_... -> turn_off_...

@heplesser heplesser requested a review from apeyser Dec 16, 2016

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 16, 2016

Contributor

@apeyser Since @jougs is unavailable at the moment, could you be the second reviewer for this one?

Contributor

heplesser commented Dec 16, 2016

@apeyser Since @jougs is unavailable at the moment, could you be the second reviewer for this one?

@jougs

Very nice work indeed. Many thanks for considering my concerns and making these extensive changes. I'll approve once my minor comments are addressed.

Show outdated Hide outdated lib/sli/nest-init.sli
/LayoutNetwork_l_a_dict
{
<< >> begin
/LayoutNetwork /deprecation_warning GetOption true eq
{
(Sli function LayoutNetwork is deprecated in NEST 3.0.) M_DEPRECATED message

This comment has been minimized.

@jougs

jougs Dec 31, 2016

Contributor

"Sli" -> "SLI"

There are some more changes like this that I won't comment on. Please search and replace all of them.

@jougs

jougs Dec 31, 2016

Contributor

"Sli" -> "SLI"

There are some more changes like this that I won't comment on. Please search and replace all of them.

Show outdated Hide outdated libnestutil/logging_event.cpp
@@ -63,6 +63,9 @@ std::ostream& operator<<( std::ostream& out, const LoggingEvent& e )
case M_INFO:
out << "[INFO] ";
break;
case M_DEPRECATED:
out << "[INFO] ";

This comment has been minimized.

@jougs

jougs Dec 31, 2016

Contributor

Wouldn't it be even more expressive if "[INFO]" was "[DEPRECATED]"? This would also allow to search the output log for such functions more easily.

@jougs

jougs Dec 31, 2016

Contributor

Wouldn't it be even more expressive if "[INFO]" was "[DEPRECATED]"? This would also allow to search the output log for such functions more easily.

Show outdated Hide outdated models/modelsmodule.cpp
"aeif_cond_alpha_RK5" );
"aeif_cond_alpha_RK5",
/*private_model*/ false,
/*deprecation_model*/ "NEST 3.0" );

This comment has been minimized.

@jougs

jougs Dec 31, 2016

Contributor

"deprecation_model" -> "deprecation_info"

@jougs

jougs Dec 31, 2016

Contributor

"deprecation_model" -> "deprecation_info"

Show outdated Hide outdated pynest/nest/lib/hl_api_connections.py
@@ -350,6 +352,8 @@ def Connect(pre, post, conn_spec=None, syn_spec=None, model=None):
@check_stack
@deprecated('', 'DataConnect is deprecated and will be removed in NEST3. Use \

This comment has been minimized.

@jougs

jougs Dec 31, 2016

Contributor

"NEST3" -> "NEST 3.0" to be consistent with the other messages

@jougs

jougs Dec 31, 2016

Contributor

"NEST3" -> "NEST 3.0" to be consistent with the other messages

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Dec 31, 2016

Contributor

@heplesser: I agree regarding the SLI examples. We can't possibly re-formulate all of them now and they will be naturally adapted when the deprecated models/functions are removed. I'm happy to accept this PR with the current current scope once my comments are addressed.

Contributor

jougs commented Dec 31, 2016

@heplesser: I agree regarding the SLI examples. We can't possibly re-formulate all of them now and they will be naturally adapted when the deprecated models/functions are removed. I'm happy to accept this PR with the current current scope once my comments are addressed.

@heplesser heplesser removed the request for review from apeyser Jan 2, 2017

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jan 2, 2017

Contributor

@apeyser @jougs has reviewed the PR now, so no need for action on your side.

Contributor

heplesser commented Jan 2, 2017

@apeyser @jougs has reviewed the PR now, so no need for action on your side.

@jougs

jougs approved these changes Jan 4, 2017

Thanks for the changes.

@jougs jougs merged commit 090346c into nest:master Jan 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stinebuu stinebuu deleted the stinebuu:fix552 branch Jan 17, 2017

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