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.

Sign up for GitHub

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

Jump to bottom

Replace Meschach by eigen #2470

Merged
merged 21 commits into from
Oct 12, 2023
Merged

Replace Meschach by eigen #2470

merged 21 commits into from
Oct 12, 2023

Conversation

alkino
Copy link
Member

@alkino alkino commented Aug 22, 2023 •
edited by jorblancoa
Loading

CI_BRANCHES:BLUECONFIGS_BRANCH=jblanco/update_references_eigen

@alkino alkino marked this pull request as draft August 22, 2023 08:25
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 9703e6b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ ed83c6b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 9c82fce -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 3f6f0ea -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 289962c -> Azure artifacts URL

@azure-pipelines
Copy link

✔️ f90d043 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 12a9a6f -> Azure artifacts URL

@codecov
Copy link

codecov bot commented Aug 24, 2023 •
edited
Loading

Codecov Report

Merging #2470 (84f07f1) into master (ba78cb8) will increase coverage by 4.37%.
The diff coverage is 98.49%.

❗ Current head 84f07f1 differs from pull request most recent head e0f5a4a. Consider uploading reports for the commit e0f5a4a to get more accurate results

@@            Coverage Diff             @@
##           master    #2470      +/-   ##
==========================================
+ Coverage   61.09%   65.46%   +4.37%     
==========================================
  Files         628      565      -63     
  Lines      121088   110943   -10145     
==========================================
- Hits        73977    72633    -1344     
+ Misses      47111    38310    -8801
Files Changed Coverage Δ
src/ivoc/matrix.cpp 58.76% <ø> (ø)
src/nrnpython/grids.cpp 76.23% <ø> (ø)
src/nrnpython/rxd.cpp 89.46% <ø> (-0.07%) ⬇️
src/nrnpython/rxd_extracellular.cpp 70.90% <ø> (+0.85%) ⬆️
test/unit_tests/matrix.cpp 97.05% <ø> (+0.06%) ⬆️
src/ivoc/ocmatrix.cpp 97.20% <98.46%> (+8.75%) ⬆️
src/ivoc/ocmatrix.h 7.69% <100.00%> (+2.74%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ a8a0058 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@alexsavulescu alexsavulescu mentioned this pull request Aug 24, 2023
Open
19 tasks
@azure-pipelines
Copy link

✔️ 978c53d -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 24dd250 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@alkino alkino changed the base branch from master to cornu/add_test_matrix August 29, 2023 11:13
@bbpbuildbot

This comment has been minimized.

Sign in to view
Base automatically changed from cornu/add_test_matrix to master August 29, 2023 12:00
@alkino alkino marked this pull request as ready for review August 29, 2023 12:22
@nrnhines
Copy link
Member

nrnhines commented Sep 6, 2023

The idea behind 9c1d070 is to orient the major axis so its 1 end is in a more positive 3-d location than the 0 end. For the
morphologies of modeldb 149100 it turns out that this was already the case for the meschach version. For this eigen pr, 3 of 4 morphologies, the soma points are reversed by the new orient function (and correspond to the meschach version). It may make sense to have this in a separate pr for easy reverting. But it will be interesting to see if this fixes the BBP and Modeldb model result differences.

@azure-pipelines
Copy link

✔️ 9c1d070 -> Azure artifacts URL

@nrnhines nrnhines mentioned this pull request Sep 7, 2023
Merged
@azure-pipelines
Copy link

✔️ 88bc143 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

NEURON ModelDB CI: launching for 88bc143 via its drop url

@bbpbuildbot

This comment has been minimized.

Sign in to view
nrnhines
nrnhines approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Import3d_GUI.hoc, I know of no way for the information from symmeig in eigen to be used to predict the legacy orientation computed by symmeig in meschach. We've added a heuristic to disambiguate the orientation. With that heuristic, symmeig and eigen generate same orientation of the major axis from the contour point data.

@azure-pipelines
Copy link

✔️ 0befaac -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@pramodk pramodk mentioned this pull request Oct 3, 2023
Merged
@azure-pipelines
Copy link

✔️ efdaaa8 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@jorblancoa jorblancoa closed this Oct 12, 2023
@jorblancoa jorblancoa reopened this Oct 12, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 14 Code Smells

No Coverage information No Coverage information
11.4% 11.4% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@azure-pipelines
Copy link

✔️ 3646c0f -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #160759 (:no_entry:) have been uploaded here!

Status and direct links:

pramodk pushed a commit that referenced this pull request Oct 12, 2023
@nrnhines
Soma contour centroid oriented so 1 end is largest positive direction. (
ba1cedc 
#2491)

A temporary pull request to allow comparison with #2470 

This was an attempt to see if this produces the same result as #2470.
But the conclusion was that this is not easy. We are still merging this
PR/change because we thought it would be helpful to have this change
into the NEURON master branch as a reference point if we ever have
to compare this change in isolation.
@pramodk pramodk merged commit 62e5368 into master Oct 12, 2023
32 of 34 checks passed
@pramodk pramodk deleted the cornu/add_eigen branch October 12, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrnhines nrnhines nrnhines approved these changes

Assignees
No one assigned
Labels
refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alkino @bbpbuildbot @nrnhines @pramodk @jorblancoa