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

Minor improvement to microcircuit example #2087

Merged
merged 6 commits into from
Jul 26, 2021

Conversation

morales-gregorio
Copy link
Contributor

As mentioned in #1207 the examples code has already improved significantly and I could only identify one place where a warning would be fitting.

Additionally I spotted one superfluous import.

Best,
Aitor

@morales-gregorio
Copy link
Contributor Author

I have changed the problematic files from #1208 in this PR too. The PBS part was removed and writing the submission file is something the user will have to do own their own.

@stinebuu stinebuu added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Jul 1, 2021
@stinebuu stinebuu requested review from jhnnsnk and stinebuu July 1, 2021 06:57
Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

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

Thanks! I have two small formatting comments, but otherwise it looks good :)

pynest/examples/Potjans_2014/helpers.py Outdated Show resolved Hide resolved
pynest/examples/Potjans_2014/network.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

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

Thanks!

@stinebuu
Copy link
Contributor

stinebuu commented Jul 1, 2021

@morales-gregorio Just noticed that GitHub actions complains about some pep8 formatting errors in network.py. Could you check that? If it is the line break I made you revert I apologize! I did not check how long the line was before I asked you to put it in one line.

@morales-gregorio
Copy link
Contributor Author

morales-gregorio commented Jul 1, 2021

Hi @stinebuu I see all the PEP8 errors refer to line breaks after a binary operator:

MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:193:72: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:195:57: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:196:66: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:198:63: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:329:46: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:423:56: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:452:71: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:459:79: W504 line break after binary operator
MSGBLD0195: [PEP8] pynest/examples/Potjans_2014/network.py:522:62: W504 line break after binary operator

Such as:

        self.num_neurons = np.round((self.net_dict['full_num_neurons'] *
                                     self.net_dict['N_scaling'])).astype(int)

But all of these happen within parentheses, so python will have no problem interpreting them :/
The line lengths seem fine :)

I would not force a \ after each of these binary operators since they are not needed for python to work and make the code cluttered. Maybe the PEP8 checker in Github actions needs to be updated ;)

@heplesser
Copy link
Contributor

heplesser commented Jul 1, 2021

@morales-gregorio No, we definitely don't want to introduce line continuation by \. But PEP8 states that when you break on a binary operator, the operator shall be at the beginning of the second line:

self.num_neurons = np.round((self.net_dict['full_num_neurons']
                             * self.net_dict['N_scaling'])).astype(int)

When fixing this, make sure you don't leave whitespace at the end of the first line or PEP8 will complain about that ;).

@morales-gregorio
Copy link
Contributor Author

Hi! I think all the tests now pass, from my side it can be merged

@jougs
Copy link
Contributor

jougs commented Jul 20, 2021

@jhnnsnk: friendly ping!

Copy link
Contributor

@jhnnsnk jhnnsnk left a comment

Choose a reason for hiding this comment

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

👍
Looks good to me. Thanks, @morales-gregorio, for the improvements (and thanks, @jougs, for the ping)!

@jougs jougs merged commit bffbca8 into nest:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: Done
5 participants