22 pre publish code tidy#27
Merged
Merged
Conversation
* Previously a FutureWarning was coming up in pytest that 'delim_whitespace' keyword in pd.read_table is deprecated, need to use sep='\s+' instead. * Deprecated in v2.2.0, see pandas-dev/pandas#55569 and https://pandas.pydata.org/docs/whatsnew/v2.2.0.html#other-deprecations for more info
* Purely stylistic as it was noticed that set-like `&` operation was computationally quite expensive. Also aids code navigation.
* Reorder class to methods-first * Doxygen-friendly header comments * Use size_t as appropriate
* Following discussion with Arni, safer to check first value rather than t < 0, especially if we start to work with demographies with K > 0
* NOTE: -Wsign-conversion warning disabled (see below) * Reorder class to methods-first. * Doxygen-friendly header comments. * Use const ref on getters to avoid copying data. * Replace commented-out diagnostic code with #def. See FIXME in CompilerWarnings.cmake about disabling -Wsign-conversion warning. This generates a lot of noise due as many counts in classes are stored as int rather than size_t. Need to review with Arni as some should remain int.
* Turn on ENABLE_TESTING and WARNINGS_AS_ERRORS for developers by default (does not affect CI). * Add file associations automatically added during build process
* Reorder class to methods-first. * Doxygen-friendly header comments. * Clang format * Warnings picked up shadowed variable in compute_expected_times * Resolve size_t/int with easiest fix where possible * Warnings picked up unused variables (e.g. `trans` line 61) * Added exception for possible data conversion loss in `breakpoints`
* Resolve size_t/int with easiest fix where possible * Comments to pick up in review whether runtime check genetic_positions is needed
* Reorder header to methods-first * Ensure all member variables initialised * Doxygen-friendly header comments * Clang format * Resolve size_t/int with easiest fix where possible * Remove C-style cast * Parenthesis to clarify conditional precedence
* Reorder header to methods-first * Clang format
* Reorder header to methods-first * Class member order initialisation corrected * Const correct `genotype` and `dump` * Doxygen-friendly header comments * Clang format * Review point whether numeric_limits infinity or max
* Reorder class to methods-first * Resolve size_t/int with easiest fix where possible * Remove commented-out Eigen-based code; may revisit later * Doxygen-friendly header comments * Clang format
* Ensure member variables initialised in order * remove `_` prefix on members (technically _ as first char is bad practice) * Separate class methods from variables * Clang format
* Reorder class to methods-first and separate variables from methods * Const correct direct getter methods * Moved coord_id_key out into cpp as it's general to all classes * Resolve size_t/int warnings * Parenthesis to confirm operator precedence in conditional * Clang format
* Reorder class to methods-first and separate variables from methods * Ensure all member variables initialised * Remove commented out member variables * Ensure member variables initialised in correct order * Resolve size_t/int warnings * Replace C-ctyle casts * Parenthesis to confirm operator precedence * Clang format * Some review comments to pick up with regard to parenthesis and commented out code
* Reorder class to methods-first and separate variables from methods * Doxygen friendly comments * Moved doxygen comments from cpp to hpp as it's common to have in latter * Ensure all member variables initialised * Ensure all member variables constructed in order * Replaced commented out debug code with #def (for review) * Replace size_t with std::size_t (not sure which headers bring in the former) * Removed unused/shado variables a_next and b_next * Resolve size_t/int warnings * Note around line 1328 - `i` variable duplicated, change to `j` (for review) * Some points for review on double to int conversion (use floor/ceil?) * Parenthesis to confirm operator precedence * Const correct where possible * Clang format
* Note that `-Wsign-conversion` is disabled to avoid changing too much code; need to review what values should be int
* -9 is now ALLELE_MISSING * -7 is now ALLELE_UNPHASED_HET
* After reviewing with Arni, this check may still be useful as currently if genetic positions are out of order then there is a potential segfault. Left enabled whilst investigating
* After review with Arni, the original reason for using unsigned short is not clear. The explicit exception for overflows captures this for now and we will do a check on static_casts in future, may be possible to just save these as ints/size_t values.
* Setting to infinity may be mathematically undefined float behaviour, where max is definitely stable.
* Reviewing this code, it looks like the segment is passed the wrong size. Needs more investigation.
* Confirmed in review this will not be needed.
* The behaviour of the double to int case is still functionally the same so OK. To clarify, this cast will be a _floor_ operation. * the .begin() usage is valid, it's a implicit range constructor. * i to j rename definitely correct; we're not sure why this has not caused problems until now.
* Checked in review, can just be done in constructor as they all have the same weighting value
* Checked in review, this is no longer needed. * Also made count_branches const.
* Apparently there were some performance issues so this was commented out in the code. It turned out the only left over uses of it were not being used, so for now it's OK to move out of the project until we can revist (theoretically it should be a performance boost, not a loss)
Closed
13 tasks
* Unlike GCC, clang needs to explicitly be told -Wno-sign-conversion * Removed unused vars * Removed implicit copy constructor TgenSegment * Removed std::move on unique ptr (copy elision warning)
* The text was adjusted as we tweaked the condition logic on the first arg, after finding potential access to -1th index during tidy.
* The previous use of `int` raised an warning in GCC that the size of boundaries could be invalid and generate a null ptr deref. Changing to `size_t` removes possibility of negative cases.
ArniFreyrG
approved these changes
May 31, 2024
Collaborator
ArniFreyrG
left a comment
ArniFreyrG
left a comment
There was a problem hiding this comment.
This is amazing! I'll make sure to stick to this code convention for future PRs
* Updating my original fix with feedback from Arni: throw exception on unhandled allele; init `z_pair` penalty/cost value to numeric max.
* This dependency comes in automatically with arg-needle-lib and can cause confusion if done beforehand, e.g. running on a platform like macos which does not yet have a pypi build. Also makes image creation faster.
fcooper8472
approved these changes
Jun 3, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See ticket for full breakdown of changes. Warnings as errors enabled apart from
-Wsign-conversionfor which a separate ticket has been raised.