Raise a meaningful error in import3d_gui.hoc for invalide soma

[WeinaJi]

Requested by users from the Blue Brain Project.
Fix #2602

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[1uc]

The reasoning is that in the next line we do:

minor.div(minor.mag)

to build a system of reference with two directions: minor and major. However, if minor.mag == 0. then we're unable to determine that the two directions. Therefore, it might be nice to throw a more meaningful error message here, since we know that semantically it's not just related to dividing something by zero, but that it's due to some degeneracy in the input contour.

[azure-pipelines]

✔️ 1ed07bd -> Azure artifacts URL

[nrnhines]

to build a system of reference with two directions: minor and major. However, if minor.mag == 0. then we're unable to determine that the two directions.

Yes. This approximates the (2-d) contour (sometimes a shape like a potato or a banana) by an ellipse with a major and minor axis.
One heuristic might be to raise an error if the minor/major ratio is less than 0.01 . Of course this won't catch errors of the form that the 3-d point author actually entered soma centroid points that are not in a straight line. In this case the analysis would just connect the first and last point to close the contour.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6f2e087) 66.19% compared to head (1ed07bd) 66.19%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2609   +/-   ##
=======================================
  Coverage   66.19%   66.19%           
=======================================
  Files         559      559           
  Lines      108937   108939    +2     
=======================================
+ Hits        72109    72112    +3     
+ Misses      36828    36827    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bbpbuildbot]

Logfiles from GitLab pipeline #169999 (:white_check_mark:) have been uploaded here!

Status and direct links:

• ✅ mac_m1_cmake_build: [cmake, ON, OFF, OFF, address]
• ✅ spack_setup
• ✅ build:nmodl
• ✅ build:neuron:nmodl:intel:legacy
• ✅ build:neuron:nmodl:intel:shared
• ✅ build:neuron:nmodl:nvhpc:acc:legacy
• ✅ build:neuron:nmodl:nvhpc:acc:shared
• ✅ build:neuron:nmodl:nvhpc:omp:legacy
• ✅ build:neuron:nmodl:nvhpc:omp
• ✅ test:neuron:nmodl:intel:legacy
• ✅ test:neuron:nmodl:intel:shared
• ✅ test:neuron:nmodl:nvhpc:acc:legacy
• ✅ test:neuron:nmodl:nvhpc:acc:shared
• ✅ test:neuron:nmodl:nvhpc:omp:legacy
• ✅ test:neuron:nmodl:nvhpc:omp

[1uc]

With ratios and relative errors the problem is often that the reference could be zero, e.g. in this case if major.mag is close to zero, the we can't rely the ratio minor.mag / major.mag to be informative. If we go the relative path, then I'd suggest:

if(major.mag < ATOL || minor.mag / major.mag < RTOL) {

If it's impossible to give a value for ATOL, then one would use the relative threshold by itself.

As for 0.01, and I'm sorry for being annoying, it feels very large. If we throw, it means we block users from simulating anything with such a thin soma. Intuitively, I was expecting a relative error of 1e-6 or smaller.

[1uc]

We never added the proposed changes. Hence what's currently merged is the check:

if(minor.mag == 0.0)

@nrnhines we're unsure if this is an oversight. Would you like us to create a new PR to check:

if(minor.mag / (major.mag + 1e-100) < 1e-6) {

[nrnhines]

So shall I throw when if (minor.mag/major.mag < 1e-6)?

My mistake, I thought that had been added. But

if(minor.mag / (major.mag + 1e-100) < 1e-6) {

is better. I guess this mistake is best corrected with a new PR.