Skip to content

Allow legacy a.b(i) syntax for 1d arrays (extend to PointProcess.var[i] and ob.dblarray[i])#2256

Merged
nrnhines merged 3 commits into
masterfrom
hines/fixarray
Mar 9, 2023
Merged

Allow legacy a.b(i) syntax for 1d arrays (extend to PointProcess.var[i] and ob.dblarray[i])#2256
nrnhines merged 3 commits into
masterfrom
hines/fixarray

Conversation

@nrnhines

@nrnhines nrnhines commented Mar 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

extends to what was meant by nrn commit a8362dc

  • Allow legacy a.b(i) syntax for 1d arrays (should be written a.b[i])

for POINT_PROCESS.var[index] and double x[index]
(not just for Vector)

@nrnhines nrnhines requested review from olupton and ramcdougal March 6, 2023 16:27
ramcdougal
ramcdougal reviewed Mar 6, 2023
View reviewed changes

@ramcdougal ramcdougal 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.

The precedent for this was already set with matrices and .x That said, is there any option of deprecating this and displaying a warning for now, converting any uses in ModelDB to [] and then removing in NEURON 10?

@azure-pipelines

azure-pipelines Bot commented Mar 6, 2023

Copy link
Copy Markdown

✔️ 3d8a580 -> Azure artifacts URL

@codecov

codecov Bot commented Mar 6, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2256 (faa0abc) into master (522c866) will increase coverage by 0.00%.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##           master    #2256   +/-   ##
=======================================
  Coverage   56.87%   56.87%           
=======================================
  Files         620      620           
  Lines      121554   121562    +8     
=======================================
+ Hits        69129    69140   +11     
+ Misses      52425    52422    -3
Impacted Files Coverage Δ
src/oc/hoc_oop.cpp 77.27% <87.50%> (+0.36%) ⬆️

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

@nrnhines

nrnhines commented Mar 6, 2023

Copy link
Copy Markdown
Member Author

The precedent for this was already set with matrices and .x That said, is there any option of deprecating this and displaying a warning for now, converting any uses in ModelDB to [] and then removing in NEURON 10?

I can't call it an option, but there is certainly a clear place in the code (those fragments that begin with if (narg) {) where one could print a warning and then eventually change to an error. As for finding all the places in ModelDB where (...) is used that should be changed to [...], it is hard to do that with grep. And, since not all code is covered, hard to do that by just looking at warning locations. The reason it's difficult to do at parse time is because at run time when a.b(i) is executed, depending on a, b may be an array or a function. But at least grep could flag all the \.[a-z][a-z0-9]_]*(.*= which, though it is a bit too big a bucket, might be worth going through.

@bbpbuildbot

bbpbuildbot commented Mar 6, 2023

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

olupton
olupton reviewed Mar 7, 2023
View reviewed changes
Comment thread src/oc/hoc_oop.cpp
@azure-pipelines

azure-pipelines Bot commented Mar 9, 2023

Copy link
Copy Markdown

✔️ faa0abc -> Azure artifacts URL

@bbpbuildbot

bbpbuildbot commented Mar 9, 2023

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@nrnhines nrnhines merged commit 0f2ece5 into master Mar 9, 2023
@nrnhines nrnhines deleted the hines/fixarray branch March 9, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ramcdougal ramcdougal ramcdougal left review comments
@olupton olupton olupton approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nrnhines @bbpbuildbot @olupton @ramcdougal