Skip to content

Remove legacy units and option of dynamic units selection#2539

Merged
pramodk merged 25 commits into
masterfrom
cornu/simplify_units
Oct 6, 2023
Merged

Remove legacy units and option of dynamic units selection#2539
pramodk merged 25 commits into
masterfrom
cornu/simplify_units

Conversation

@alkino

@alkino alkino commented Sep 26, 2023

Copy link
Copy Markdown
Member

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

azure-pipelines Bot commented Sep 26, 2023

Copy link
Copy Markdown

✔️ 1509226 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines

azure-pipelines Bot commented Sep 26, 2023

Copy link
Copy Markdown

✔️ 2395d08 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines

azure-pipelines Bot commented Sep 26, 2023

Copy link
Copy Markdown

✔️ 092734a -> Azure artifacts URL

@codecov

codecov Bot commented Sep 26, 2023 •
edited
Loading

Copy link
Copy Markdown

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
@alkino alkino added the nrn-modeldb-ci-nightly Launch external ModelDB CI label Sep 26, 2023
@github-actions github-actions Bot removed the nrn-modeldb-ci-nightly Launch external ModelDB CI label Sep 26, 2023
@github-actions

github-actions Bot commented Sep 26, 2023

Copy link
Copy Markdown
Contributor

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

iomaganaris
iomaganaris reviewed Sep 27, 2023
View reviewed changes

@iomaganaris iomaganaris left a comment

Copy link
Copy Markdown
Member

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

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

This comment has been minimized.

Sign in to view
@azure-pipelines

azure-pipelines Bot commented Sep 27, 2023

Copy link
Copy Markdown

✔️ ad693fb -> Azure artifacts URL

@azure-pipelines

azure-pipelines Bot commented Sep 28, 2023

Copy link
Copy Markdown

✔️ 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
Comment thread src/nrnoc/init.cpp
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines

azure-pipelines Bot commented Sep 30, 2023

Copy link
Copy Markdown

✔️ 384c778 -> Azure artifacts URL

pramodk
pramodk reviewed Oct 4, 2023
View reviewed changes

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

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?

Comment thread src/nrnoc/init.cpp Outdated
Comment thread src/nrnoc/init.cpp
ohm314
ohm314 reviewed Oct 5, 2023
View reviewed changes
Comment thread docs/hoc/modelspec/programmatic/mechanisms/nmodl.rst Outdated
ohm314
ohm314 reviewed Oct 5, 2023
View reviewed changes
Comment thread docs/python/modelspec/programmatic/mechanisms/nmodl.rst Outdated
@azure-pipelines

azure-pipelines Bot commented Oct 5, 2023

Copy link
Copy Markdown

✔️ 1921c70 -> Azure artifacts URL

@alkino

alkino commented Oct 5, 2023

Copy link
Copy Markdown
Member Author

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

azure-pipelines Bot commented Oct 5, 2023

Copy link
Copy Markdown

✔️ 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

azure-pipelines Bot commented Oct 6, 2023

Copy link
Copy Markdown

✔️ b69138b -> Azure artifacts URL

@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2023

Copy link
Copy Markdown

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

azure-pipelines Bot commented Oct 6, 2023

Copy link
Copy Markdown

✔️ db56d37 -> Azure artifacts URL

@pramodk pramodk enabled auto-merge (squash) October 6, 2023 12:19
@pramodk pramodk dismissed iomaganaris’s stale review October 6, 2023 12:19

already addressed

@pramodk pramodk merged commit b3aa3d0 into master Oct 6, 2023
@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

bbpbuildbot commented Oct 6, 2023

Copy link
Copy Markdown
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