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

Remove legacy units and option of dynamic units selection #2539

Merged
merged 25 commits into from
Oct 6, 2023
Merged

Remove legacy units and option of dynamic units selection #2539

merged 25 commits into from
Oct 6, 2023

Conversation

alkino
Copy link
Member

@alkino alkino commented Sep 26, 2023

In 2018, units definition have been reworked.
It leads to difference of results depending of the system we use.
In nrn we were able to choose the legacy or the modern units.
This patch remove this system and makes nrn use only the modern units.

alkino and others added 2 commits September 26, 2023 15:19
@alkino
No more dynamic or legacy units
0317a81
@adamjhn
Update test rxd test data.
1509226 
Also fix some tests that fail when saving the data.
@azure-pipelines
Copy link

✔️ 1509226 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 2395d08 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 092734a -> Azure artifacts URL

@codecov
Copy link

codecov bot commented Sep 26, 2023 •
edited
Loading

Codecov Report

Merging #2539 (62daf4d) into master (66a7c0e) will decrease coverage by 0.06%.
Report is 1 commits behind head on master.
The diff coverage is 68.96%.

❗ Current head 62daf4d differs from pull request most recent head b69138b. Consider uploading reports for the commit b69138b to get more accurate results

@@            Coverage Diff             @@
##           master    #2539      +/-   ##
==========================================
- Coverage   61.53%   61.48%   -0.06%     
==========================================
  Files         625      625              
  Lines      119219   119120      -99     
==========================================
- Hits        73362    73239     -123     
- Misses      45857    45881      +24
Files Coverage Δ
share/lib/python/neuron/rxd/constants.py 80.00% <ø> (-1.82%) ⬇️
share/lib/python/neuron/rxd/rxd.py 83.86% <100.00%> (-0.13%) ⬇️
src/coreneuron/apps/main1.cpp 72.22% <ø> (-0.21%) ⬇️
src/coreneuron/io/global_vars.cpp 72.15% <ø> (+0.20%) ⬆️
src/nrniv/kschan.h 62.02% <ø> (ø)
...rniv/nrncore_write/callbacks/nrncore_callbacks.cpp 93.68% <ø> (+0.29%) ⬆️
src/nrniv/nrncore_write/io/nrncore_io.cpp 76.67% <ø> (-0.09%) ⬇️
src/nrniv/nrncore_write/utils/nrncore_utils.cpp 54.26% <ø> (-0.56%) ⬇️
src/nrnoc/eion.cpp 80.27% <100.00%> (-2.78%) ⬇️
src/nrnpython/nrnpy_hoc.cpp 70.91% <ø> (+0.12%) ⬆️
... and 7 more

... and 11 files 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
@github-actions
Copy link
Contributor

NEURON ModelDB CI: launching for 092734a via its drop url

iomaganaris
iomaganaris reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@iomaganaris iomaganaris left a comment

Choose a reason for hiding this comment

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

Some small suggestions for improvement
Overall LGTM if we decide that's the way to go

share/lib/python/neuron/rxd/constants.py Show resolved Hide resolved
share/lib/python/neuron/rxdtests/do_test.py Show resolved Hide resolved
src/oc/nrnunits_modern.h Outdated Show resolved Hide resolved
test/rxd/test_currents.py Outdated Show resolved Hide resolved
test/rxd/test_currents.py Outdated Show resolved Hide resolved
@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ ad693fb -> Azure artifacts URL

@azure-pipelines
Copy link

✔️ da0aba2 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
iomaganaris
iomaganaris previously requested changes Sep 28, 2023
View reviewed changes
src/nrnoc/init.cpp Outdated Show resolved Hide resolved
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ 384c778 -> Azure artifacts URL

pramodk
pramodk reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

@alkino : is it possible to add a test that Ioannis suggested? i.e. if someone tries to set the legacy unit the execution should fail. Or, it's difficult to test because we call abort?

src/nrnoc/init.cpp Outdated Show resolved Hide resolved
src/nrnoc/init.cpp Show resolved Hide resolved
@azure-pipelines
Copy link

✔️ 1921c70 -> Azure artifacts URL

@alkino
Copy link
Member Author

alkino commented Oct 5, 2023

So it fails even with expect_err, everyone is happy.

@alkino alkino closed this Oct 5, 2023
@alkino alkino reopened this Oct 5, 2023
@azure-pipelines
Copy link

✔️ 4df612a -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ b69138b -> Azure artifacts URL

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@azure-pipelines
Copy link

✔️ db56d37 -> Azure artifacts URL

@pramodk pramodk enabled auto-merge (squash) October 6, 2023 12:19
@pramodk pramodk merged commit b3aa3d0 into master Oct 6, 2023
33 of 34 checks passed
@pramodk pramodk deleted the cornu/simplify_units branch October 6, 2023 12:45
@pramodk pramodk changed the title Remove dynamic units and legacy units Remove legacy units and option of dynamic units selection Oct 6, 2023
@bbpbuildbot
Copy link
Collaborator

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

Status and direct links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ohm314 ohm314 ohm314 left review comments

@nrnhines nrnhines nrnhines left review comments

@iomaganaris iomaganaris iomaganaris left review comments

@pramodk pramodk pramodk approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@alkino @bbpbuildbot @pramodk @ohm314 @nrnhines @iomaganaris @adamjhn