Closed nickjcroucher closed 3 years ago
Also bump version in __init__
Web tests failing. Let's consider changing them or removing them
Also bump version in
__init__
Done in a67de16.
For future reference - the default values for C++ functions should be in python_bindings.cpp
, not boundary.hpp
, as otherwise the tests fail. Restored in 1f0946e.
For future reference - the default values for C++ functions should be in
python_bindings.cpp
, notboundary.hpp
, as otherwise the tests fail. Restored in 1f0946e.
Weird, not what I'd expect! What's the error?
For future reference - the default values for C++ functions should be in
python_bindings.cpp
, notboundary.hpp
, as otherwise the tests fail. Restored in 1f0946e.Weird, not what I'd expect! What's the error?
Traceback (most recent call last):
File "/Users/ncrouche/Documents/PopPUNK/PopPUNK/test/test-refine.py", line 71, in <module>
poppunk_refine.generateTuples([int(x) for x in assign0_res], -1, True))
TypeError: generateTuples(): incompatible function arguments. The following argument types are supported:
1. (assignments: List[int], within_label: int, self: bool, num_ref: int, int_offset: int) -> List[Tuple[int, int]]
It does not like the input argument format it seems - guess this is a bit of an atypical call though.
For future reference - the default values for C++ functions should be in
python_bindings.cpp
, notboundary.hpp
, as otherwise the tests fail. Restored in 1f0946e.Weird, not what I'd expect! What's the error?
Traceback (most recent call last): File "/Users/ncrouche/Documents/PopPUNK/PopPUNK/test/test-refine.py", line 71, in <module> poppunk_refine.generateTuples([int(x) for x in assign0_res], -1, True)) TypeError: generateTuples(): incompatible function arguments. The following argument types are supported: 1. (assignments: List[int], within_label: int, self: bool, num_ref: int, int_offset: int) -> List[Tuple[int, int]]
It does not like the input argument format it seems - guess this is a bit of an atypical call though.
Looks like it's missing two arguments num_ref and int_offset. You can have these defaults set in the m.def
part of python bindings and they appear as they defaults to the python call. C++ defaults (so if using the function from elsewhere in the C++ not just the python call) should still go in the .hpp I believe. It's ok to have both of these, as the defaults in the m.def
part have a different meaning
Looks like it's missing two arguments num_ref and int_offset. You can have these defaults set in the
m.def
part of python bindings and they appear as they defaults to the python call. C++ defaults (so if using the function from elsewhere in the C++ not just the python call) should still go in the .hpp I believe. It's ok to have both of these, as the defaults in them.def
part have a different meaning
I don't really mind, I assumed we were prioritising only specifying the defaults once (guess if there were a difference, the C++ ones would override the python ones?). Can add the defaults back into the .hpp
if useful, let me know.
I don't really mind, I assumed we were prioritising only specifying the defaults once (guess if there were a difference, the C++ ones would override the python ones?). Can add the defaults back into the
.hpp
if useful, let me know.
I think I misunderstood when I saw the .cpp extension. If you've got defaults set in the m.def section only that's totally fine
I think I misunderstood when I saw the .cpp extension. If you've got defaults set in the m.def section only that's totally fine
Got it, thanks! - now we've made this decision, apologies in advance for inevitably forgetting about this discussion and putting unhelpful defaults into .cpp
and/or .hpp
in the future.
Fixed the web tests now - the only bit that might need investigation is the graph weights, as they are set to be true in args.txt
, but are not actually stored in the graph any more. I've overridden this in the tests themselves, but if the visualisation needs the weights, we may need to rethink some of the network processing.
Passing tests now, updated tests on SPARC still seem to be working:
One more thought - should the info.py
script report the strand-specific k-mer type of the database, and if so, would that change the sketchlib version requirement?
Is this ready for re-review now?
One more thought - should the
info.py
script report the strand-specific k-mer type of the database, and if so, would that change the sketchlib version requirement?
Yes, that would be a useful addition. The best way to deal with these attributes which may or may not be present depending on version is to try and read them and potentially handle the error, or check for presence of the attribute name in the keys and set default/unknown if not there.
Great, added the check in 5facf69. Yes, now ready for re-review. Thanks for the very helpful comments!
Happy to merge this when the tests have passed then!
Fixes and upgrades to refinement, particularly with the individual refinement and GPU libraries. Examples of output with the SPARC dataset:
CPU output: https://microreact.org/project/8i4QSPr6J1t9WDJCtN7xXy
GPU output: https://microreact.org/project/7CEYuuPk5c13JMum3QdM6R
Fixes include:
updating reference graphs
loading GPU libraries and updating GPU tests
isolate name processing
storing, updating and visualising individual refinement graphs