Implemented network generation. #27
Conversation
pydepsi/network.py
Outdated
| min_length: float, minimum length of any generated arc. | ||
| max_length: float, maximum length of any generated arc or None. | ||
| max_links: int, maximum number of arcs per node for the redundant method. | ||
| num_groups: int, number of parts to split the orientations around each node into for the redundant method. |
There was a problem hiding this comment.
Please rename to num_parts (like in original implementation, indicates 'partitions', i.e., sub-division.
There was a problem hiding this comment.
I renamed the 'groups' arguments and variables to 'partitions'.
I generally don't like shortening terms unless they are extremely common. In code, clarity usually trumps brevity.
However, I could still change this to 'parts' if you still prefer this.
There was a problem hiding this comment.
Thanks @vanlankveldthijs for the nice implementation! I have some comments but they are all very minor.
Apart from the in line comments, can you also add some unit tests to the functions? Thanks!
pydepsi/network.py
Outdated
| arcs: list of pairs, point indices describing the adjacent nodes. The pairs are sorted, as is the list. | ||
| """ | ||
| if min_length <= 0: | ||
| print(f"min_length must be positive (currently: {min_length})") |
There was a problem hiding this comment.
I like the screen feedbacks! For consitency and good practice, consider using Python logger? This allow the user to access these information later.
You can find an example of using logger for warning in stmtools io module. Here we can simply using info.
There was a problem hiding this comment.
I replaced the print messages by logger messages.
However, testing shows that info messages do not show in the example notebook when an invalid argument is provided. Instead the code will fail because of incorrect return values without any message with the reason.
When using logger.warning, the example notebook displays these messages as desired.
I set them to 'warning' instead of 'info'. Let me know if I should make them 'info' regardless.
There was a problem hiding this comment.
Hi Thijs @vanlankveldthijs , I think we shoud still use info, since the information here are not warning messages.
By default, Jupyter kernel uses warning as default level. I guess that's why the info does not should up. Maybe try the following setting in your Notebook? I think they will enable displaying the INFO messages.
import logging
logging.basicConfig(level=logging.INFO)There was a problem hiding this comment.
@vanlankveldthijs sorry given another look in the context, I realized maybe the information here should be error?maybe let update that?
Also adjusted comments to reflect this change.
|
Added unit tests for network.py To enable these, I had to add some test data to the repository. |
Thanks Thijs! I replied in the logging comment. However I did not see the unit tests. Maybe you forgot to push them? (or maybe I missed them?) |
I may have forgot to push the tests. They should be there now :) |
rogerkuou
force-pushed
the
16_generate_arcs
branch
from
9afbca8 to
4553760
Compare
|
Hi @vanlankveldthijs, thanks! The unit test are all good! I did one change myselft to merge your test data file into one chunk to reduce the number of files. In order to eliminate the files from history, I overrite your last commit. I have one more minor comment on the logging. I suggest to use |
Resolves issue #16
Can switch between Delaunay and 'redundant' networks.
Also comes with a demo notebook, but this does not have extensive documentation or explanation (yet).