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

Move ConnPlotter code from NEST to a separate repository #1

Closed
wants to merge 2 commits into from

Conversation

pnbabu
Copy link
Collaborator

@pnbabu pnbabu commented Sep 1, 2021

Fixes #1971

This PR moves all the files related to ConnPlotter from nest-simulator repo.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. I'm wondering a bit if the toplevel Python files (other than setup.py, that is) should go into their own src or nest-connplotter directory instead of being toplevel.

I'd also like to hear @heplesser's feedback as original author before merging

@jougs jougs requested a review from heplesser September 1, 2021 18:03
@jougs
Copy link
Contributor

jougs commented Sep 1, 2021

And I think that examples/__init__.py can go. It does not have anything useful.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This move is fine with me. "examples" is currently set up as a package, thus examples/__init__.py. But having it as a package does not really make sense, so the __init__.py should be removed. This has consequences:

  • connplotter_tutorial.py relies on examples being a package, but that should be easily fixed.
  • Also setup.py treats examples as a package and needs to be adjusted to ensure that the examples will be installed.

Finally, the tutorial is in a very outdated literal programming format, it would be great to convert it to a notebook or our standard style for examples.

@pnbabu
Copy link
Collaborator Author

pnbabu commented Sep 2, 2021

Removed examples/__init__.py and updated the tutorial and setup.py accordingly.

I am trying to run the tutorial locally on my machine and it is taking a long time to run. I will investigate what is taking time. @heplesser When you say standard style, are you referring to pynest/examples in the NEST repo?

@jougs jougs mentioned this pull request Sep 7, 2021
@Helveg
Copy link
Contributor

Helveg commented Sep 7, 2021

Oh I think I did some duplicate things in #2. Some things that should be taken from there:

  • import matplotlib.font_manager, not all version of matplotlib import this when mpl is imported and an AttributeError can occur (happened to me on v3.4.3)
  • In setup.py:
    • Replace from distutils.core with from setuptools; distutils is problematic and deprecated (will be removed in Python 3.12). distutils packages cannot be uninstalled by pip for example.
    • Remove the package_dir argument and place source code files into a ConnPlotter or nest_connplotter directory, then just give packages=["nest_connplotter"] for simplified and conventional packaging.
  • Add a .gitignore

Helveg added a commit to Helveg/nest-connplotter that referenced this pull request Sep 7, 2021
@jougs
Copy link
Contributor

jougs commented Sep 7, 2021

Work and discussion continues in #2.

@jougs jougs closed this Sep 7, 2021
Helveg added a commit to Helveg/nest-connplotter that referenced this pull request Sep 8, 2021
Co-authored-by: Pooja Babu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move ConnPlotter from NEST code base to separate package
4 participants