[CODE] Refactoring base.py sort model to be cleaner and easier to read.

[ughstudios]

• Some of the code in base.py is very bad and will probably need to be changed again. However, I added type hints and removed some bad code and cleaned up a bit of the logic.

#1278

I wrote a comment on this last pull request.

Checklist

• Keep pull requests small so they can be easily reviewed.
• Update the documentation, please follow the numpy style guide.
• Update tests.
• Categorize the PR by setting a good title and adding one of the labels:
  bug, feature, ui, change, documentation, breaking, ci
  as they show up in the changelog.
• Link this PR to related issues by using closing keywords.
• Add a milestone to the PR (and related issues, if any) for the intended release.
• Request a review from another developer.

[cmutel]

This is a good first step but the logic here is still broken (not your fault). The earlier call replaces only NaN with an empty string. Assuming that missing values are zero is maybe OK, but WRT sorting I would put missing values (i.e. ones which don't have an explicit zero) below everything, including below negative numbers.

The basic problem is that earlier in the code there is an explicit inclusion of string types into a numeric array. Once this happens there is not reliable sorting. Or, in other words, this whole function should not need to exist.

[marc-vdm]

The basic problem is that earlier in the code there is an explicit inclusion of string types into a numeric array.

Agree, plan is to update how results are processed after 2.10 sometime anyway, so this was not a priority.

There's a related issue here: #887

[marc-vdm]

Thanks for the PR! See separate comments for requested changes. If you have questions/comments, let me know.

In addition to requested code-changes, please update the name of your PR to something more easily understandable so we can understand/retrace from merge history/git blame what's happened and why, see the pull-requests section of CONTRIBUTING.MD for some pointers.

Finally, please don't remove the PR checklist when opening PRs, the checklist makes it easier for us to make sure the administrative side is roughly in order.

Thanks!

[ughstudios]

The basic problem is that earlier in the code there is an explicit inclusion of string types into a numeric array. Once this happens there is not reliable sorting. Or, in other words, this whole function should not need to exist.

Yeah I agree with you. There's something that is feeding incorrect data into the model. I'm going to make these changes that Marc requested. I'll also look into seeing where we could make a "real" fix to the issue.

[marc-vdm]

I'll also look into seeing where we could make a "real" fix to the issue.

I'd suggest keeping this low on your priorities, we have on the planning to make major changes to results.

[marc-vdm]

@ughstudios We are looking to make a 2.10 release soon-ish. Will you finish the remaining task or should we close this?

[coveralls]

coverage: 56.078% (-0.02%) from 56.101%
when pulling c104a57 on ughstudios:sorting_code_cleanup
into ee24c15 on LCA-ActivityBrowser:main.