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

refactor: use class Edge instead of complicated type annotation #591

Merged

Conversation

jnussbaum
Copy link
Collaborator

@jnussbaum jnussbaum commented Oct 25, 2023

The complicated type annotation list[tuple[int, int, XMLLink | ResptrLink]] is replaced by a new model: class Edge in models.py

@jnussbaum jnussbaum self-assigned this Oct 25, 2023
@jnussbaum jnussbaum changed the title use class Edge instead of complicated type annotation refactor: use class Edge instead of complicated type annotation Oct 25, 2023
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

I generally like the changes. How confident are you that this does not have a negative performance impact for large datasets?

src/dsp_tools/analyse_xml_data/models.py Outdated Show resolved Hide resolved
@jnussbaum
Copy link
Collaborator Author

I generally like the changes. How confident are you that this does not have a negative performance impact for large datasets?

How could it have a negative impact? I didn't think about it, but I cannot think of a way how it impacts performance negatively.

@BalduinLandolt
Copy link
Collaborator

I generally like the changes. How confident are you that this does not have a negative performance impact for large datasets?

How could it have a negative impact? I didn't think about it, but I cannot think of a way how it impacts performance negatively.

Just one example that struck my eye:

graph.add_edges_from([e.as_tuple() for e in edges])

This loop was previously not needed, now it's there. And maybe there is more like that, I did not search systematically. If it adds up to some seconds for a big upload, we don't need to care... but it would be good to be sure.

Could you compare how long the graph analysis routine takes on the SGV XML, between the current main branch and yours with all the changes. Then we don't need to guess/hope, but simply know it.

@jnussbaum jnussbaum merged commit 1c9d3e9 into wip/refactor-graph-analysing-2 Oct 25, 2023
6 checks passed
@jnussbaum jnussbaum deleted the wip/refactor-graph-analysing-3 branch October 25, 2023 13:23
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.

None yet

2 participants