This page is intended for comments, suggestions and ideas regarding general style and practice or specific bits of code in the project.
Please sign suggestions with your username (append the "@" character to the beginning to have it URLified).
Once a comment has been implemented, add a note to the relevant section, ideally containing a link to the relevant commit(s).
Code review meetings
This section collects links to the material discussed in code review meetings, which are currently held Thursdays 10am UK time.
30.07.2019, Optimise network graphs to create dense matrices, by Lee
11.07.2019, Smarter outputs for Finesse by Sean: https://git.ligo.org/finesse/finesse3/blob/master/examples/code_review/smarter_simulation_outputs.ipynb
- Frequencies in Finesse by Daniel: https://git.ligo.org/finesse/finesse3/blob/master/examples/code_review/Frequencies.ipynb
- and CLI branch for Pykat by Sean: https://git.ligo.org/finesse/pykat/merge_requests/5#note_103172
- 20.06.2019, Port and node system by Sam: https://git.ligo.org/finesse/finesse3/blob/master/examples/code_review/port_node_system.ipynb
Code style and linting tools
@ssl: the README suggests that code style can be fixed later. This is probably worth doing sooner rather than later to avoid having to change too much. I would suggest following PEP-8 for Python code and PEP-7 for C code. Tools such as autopep8 are available to enforce rules, but it may be worth writing a custom configuration that we're all happy with.
Another suggestion would be to use pylint either instead of or in parallel to some style checking tool. Periodically using this on the source code to remove unused imports, suggest code changes, etc. is good housekeeping. Again, it would probably need a project-specific configuration file. It can be integrated into Visual Studio Code and other editors.
@ddb: 1/5/18 Decided to try out pylint, asked @sjrowlinson to take a look at whether we can incorporate this into the Finesse project
@ssl: regarding the tests in
finesse/tests/: instead of adding
sys.path.append('../') at the top of a test file, it might be worth assuming the user has the relevant imports already on their path. This can be enforced by making an editable installation of the project via
$ pip install -e path/to/repository/containing/setup.py
@pjj 03/05/18 This seems like a cleaner way of doing things, pushed changes to git & put a note in the readme.
@ssl: a nice library for testing is the built-in Python unittest module. From experience I'd suggest avoiding writing exhaustive unit tests right now in case the API changes as development continues, but it is already pretty useful for comparing outputs. It is especially useful for this kind of project in combination with Numpy's
allclose method which can compare matrices to be equal within tolerance.
Another potentially useful testing library that I've heard good things about is Hypothesis. This tries to find edge cases that make functions fail.
@ssl: Finesse is primarily a library. Only the front end
kat interface should write (print) to the output by default. All printing within the library should be via some logger class, and the front end should then decide whether to show it. During development, users can switch logging levels on to see messages again, but the key thing is that it can easily be switched off / quieter on a per-user basis. (And it's also useful for nightly testing - can trigger e.g. email alerts when an error is picked up).
Here is a nice guide to the reason for using logging and how to use Python's built-in library. I've implemented it in some other projects and would be happy to set this up for this project.
@ddb: 1/5/18 Decided that the Logging module is a good way to go now so we get used to using it earlier rather than later.