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

Hypergraph refactor #77

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Hypergraph refactor #77

wants to merge 8 commits into from

Conversation

tomichels
Copy link

No description provided.

added tests for different types of hypergraphs
and added fix for negative vertices
Comment on lines +433 to +448
node_size (optional): int
Change the size of the drawen nodes
pattern_size (optional): range
Only draw patterns that are in range of pattern_size
num_neurons: None or int
If None, only the neurons that are part of a pattern are shown. If an
integer is passed, it identifies the total number of recorded neurons
including non-pattern neurons to be additionally shown in the graph.
Default: None
must_involve_neuron (optional) : int
Highlight pattern which includes neuron x
node_color (optional) : String
change the color of the nodes

node_linewidth (optional) : int
change the line width of the nodes
Copy link
Member

Choose a reason for hiding this comment

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

I think the order of the individual options seems a bit random. I think users will expect first the options that control what is shown (e.g., number of neurons), then how its shown (e.g., node color).

Comment on lines -465 to 477
plt.show()
viziphant.patterns.plot_patterns_hypergraph(patterns)

Copy link
Member

Choose a reason for hiding this comment

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

What was the reason to remove the fig = and plt.show() parts?

Comment on lines +107 to +112
if isinstance(self.vertices[0], int):
max_vertex = max(max(hyperedge) for hyperedge in self.hyperedges)
else:
max_vertex = 0
if isinstance(max_vertex, str):
max_vertex = 0
Copy link
Member

Choose a reason for hiding this comment

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

Can you include comments on the logic of these if statements? Why int and str?

Copy link
Member

Choose a reason for hiding this comment

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

I assume you need this to get unique IDs for the pseudo vertecies?

Comment on lines +47 to +55

node_size (optional) : int
Size of the nodes in the Hypergraphs

node_color (optional) : String
change the color of the nodes

node_linewidth (optional) : int
change the line width of the nodes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
node_size (optional) : int
Size of the nodes in the Hypergraphs
node_color (optional) : String
change the color of the nodes
node_linewidth (optional) : int
change the line width of the nodes
node_size (optional) : int
Size of the nodes in the Hypergraphs
node_color (optional) : String
change the color of the nodes
node_linewidth (optional) : int
change the line width of the nodes

nx.dijkstra_path_length(graph, 1, 2), nx.dijkstra_path_length(graph, 1, 4)
)

# Two Hypergraphs with the same data should be equal
Copy link
Member

Choose a reason for hiding this comment

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

I think all unit tests should have such a sentence as a comment that briefly outlines the idea of the test.

Comment on lines +23 to +37
# self.spiketrains = elephant.spike_train_generation.compound_poisson_process(
# rate=5 * pq.Hz,
# amplitude_distribution=[0] + [0.98] + [0] * 8 + [0.02],
# t_stop=10 * pq.s,
# )
# self.patterns = elephant.spade.spade(
# spiketrains=self.spiketrains,
# binsize=1 * pq.ms,
# winlen=1,
# min_spikes=2,
# n_surr=100,
# dither=5 * pq.ms,
# psr_param=[0, 0, 0],
# output_format="patterns",
# )["patterns"]
Copy link
Member

Choose a reason for hiding this comment

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

Why are these lines commented out -- forgotten or kept for a potential later unit test? In the latter case, please introduce a TODO comment. Otherwise, in the former case, please remove this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants