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

lint fixes + improvement of some older code #32

Merged
merged 5 commits into from
Nov 10, 2020
Merged

Conversation

arnaudon
Copy link
Collaborator

If the detected apical point is larger than the last bifurcation, the apical points it not a valid one. Forcing it to be 0.9* distance to last bif makes it a valid one. The choice of 0.9 is arbitrary, and originated because using a factor of 1 did not work later with TNS, possbily due to floating point precision. We can but a smaller fraction than 0.9.

@arnaudon
Copy link
Collaborator Author

the failing CI seems to not be my fault...

@wizmer
Copy link
Contributor

wizmer commented Oct 16, 2020

Can you add a test ? :)
Are you sure it works. Because it will call:

min(last_bif, None)

and I don't think this is valid.
No ?

@arnaudon
Copy link
Collaborator Author

This never happens, we always get a float, as said in the docstring.

tmd/Topology/analysis.py Outdated Show resolved Hide resolved
tmd/Topology/methods.py Outdated Show resolved Hide resolved
tmd/Topology/methods.py Outdated Show resolved Hide resolved
@arnaudon arnaudon changed the title apical_point cannot be larger than last bif lint fixes + improvement of some older code Oct 26, 2020
@arnaudon
Copy link
Collaborator Author

The original aim of this PR is no more, only the lint and old code updates are left. The original PR was implemented elsewhere. @lidakanari , I will let you merge this!

@lidakanari lidakanari merged commit dc63736 into master Nov 10, 2020
@adrien-berchet adrien-berchet deleted the apical_point_fix branch July 6, 2021 07:25
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

5 participants