Fix forwarding arguments for flagser #21
Conversation
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
std::to_string(<something>).c_str() creates a temporary reference Which was lost in named_arguments, due to that max-dim and min-dim where always equal to 0 ... Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
|
@MonkeyBreaker thanks for the fixes! However, can we please also add those tests before merging? It is too important as we have seen, and would reduce review time somewhat. |
|
Sure, I'm preparing and verifying them. |
|
@MonkeyBreaker thanks. Sure, take your time! |
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
pyflagser/tests/test_flagser.py
Outdated
| def test_filtrations(flag_file): | ||
| """ | ||
| Testing all filtrations available for dataset d5.flag | ||
| It was necessary to disable edge filtrations because of assumption that the |
There was a problem hiding this comment.
Could you explain this in more detail (not in docstring, just here for now)? What does "disabling edge filtrations" look like in the C++ code?
There was a problem hiding this comment.
Well when testing with the data of the d5.flag, vertex_degree filtration produces a segmentation fault in C++, so I assumed it was related to the warning in the documentation about the filtration. And I copy/pasted on the docstring to explain the reason not all filtrations are tested.
But, the example I was working earlier this week, with data generated by squareform(pdist(np.random.random((5, 2)))). Filtration vertex_degree does work.
Am I wrong ?
There was a problem hiding this comment.
@MonkeyBreaker now you're talking about vertex_degree filtrations, but in the docstrings you are referring to "edge filtrations". Could you explain the relation?
There was a problem hiding this comment.
From the documentation:
filtration : string, optional, default:
'max'
Algorithm determining the filtration. Warning: if an edge filtration is
specified, it is assumed that the resulting filtration is consistent,
meaning that the filtration value of every simplex of dimension at
least two should evaluate to a value that is at least the maximal value
of the filtration values of its containing edges. For performance
reasons, this is not checked automatically.
When I was performing tests, I encountered segmentation faults running flagser on d5.flag with vertex_degree filtration, on the bindings and with the C++ implementation. Trying to understand why this could happen, I assumed it was related to the remark from the documentation.
|
Result were controlled with the c++ implementation, same result obtained. |
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Fix windows compilation, disable AVX instructions Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
There was a problem hiding this comment.
@MonkeyBreaker are you happy to merge this once pipelines succeed?
|
Sure, I'm creating the issues, and if I have some time, try to explore why the segmentation fault is raised. |
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Fixes multiples issues where forwarding arguments to
flagser.zerofiltration.max-dimandmin-dimwere always equal to 0.Any other comments?