-
Notifications
You must be signed in to change notification settings - Fork 371
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
Rename Parameter class in Topology to TopologyParameter #408
Conversation
@@ -24,6 +24,7 @@ | |||
import nest | |||
import nest.topology as tp | |||
import matplotlib.pyplot as plt | |||
from mpl_toolkits.mplot3d.axes3d import Axes3D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used somewhere in the file? If it is, why was this import
not here before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required to enable 3d projection here. I am not sure why it was not needed before. That may be due to internal changes in matplotlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
Why did you re-add all the figures? For me they look the same, with only very minor shifts probably due to another version of some font library. Can you please remove them? The code changes are trivial and look good to me. 👍 once the images are gone and my other comment is addressed. |
@jougs I committed all the figure files because they were recreated with the modified script and topology code. Incidentally, this fixes #300: Fig 3.6. is now back in the PDF. But I agree that the figures should probably not be committed, just the final manuscript PDF. I will create a new issue for that change. I'd appreciate merging this PR soon, since #352 depends on it. |
Agreed. As this is just a simple rename otherwise, I'll merge without second review. |
The renaming is motivated by #352 and the need for a
Parameter
class in NEST. As an extra, it fixes a little detail in the scripts used to build the topology user manual. The main point of renaming in topology is that theParameter
class to be introduced in nestkernel will be the general parameter class for the future.@jakobj @jougs Would you take a look---it is really just renaming.