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.

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

Dynam units #746

Merged
merged 28 commits into from
Oct 15, 2020
Merged

Dynam units #746

merged 28 commits into from
Oct 15, 2020

Conversation

nrnhines
Copy link
Member

@nrnhines nrnhines commented Sep 28, 2020

Closes #599
Closes #324
Closes #519
Linked to neuronsimulator/progref-py#30

@nrnhines nrnhines added this to the Release v8.0a milestone Sep 28, 2020
@nrnhines nrnhines marked this pull request as draft September 29, 2020 00:50
@nrnhines nrnhines marked this pull request as ready for review October 1, 2020 23:16
@pramodk
Copy link
Member

pramodk commented Oct 4, 2020

@nrnhines : is this ready for review or still any work is pending?

@nrnhines
Copy link
Member Author

nrnhines commented Oct 4, 2020

is this ready for review

Getting very close. Just want to consult with @adamjhn a bit about whether there is a place in rxd that is called by finitialize so it can test whether h.nrnunit_use_legacy() has changed and if so, update things that depended on NA (avogadro's number).

adamjhn and others added 3 commits October 6, 2020 16:04
… functions. (#753)

* Stop caching constants in rxd and moved calls to constants.NA into functions.
This avoids storing constants on import which will be inconsistent if you then switch to using the legacy values.

* Check for changes to nrnunit_use_legacy and reinitialize potentially relevant parts of the rxd model.
@nrnhines
Copy link
Member Author

nrnhines commented Oct 6, 2020

@pramodk @alexsavulescu This is ready for review. Note: BBP models may need export NRNUNIT_USE_LEGACY=1 to retain quantitative identity with older versions of NEURON and current versions of CoreNEURON

LegacyFR no longer a configure option.
Copy link
Member

@alexsavulescu alexsavulescu left a comment

Choose a reason for hiding this comment

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

LGTM.
Regarding NRN_ENABLE_LEGACY_FR, wouldn't it be more convenient to have it OFF by default?

Modern constants updated to codata2018 values.
The units test wasn't activated since it did not begin with 'test_'
h.Avogadro_constant available from hoc.
@nrnhines
Copy link
Member Author

@ramcdougal @adamjhn To aid in single sourcing the definition of modern physical constants, I went ahead with your original suggestion and introduced h.Avogadro_constant which returns the modern or legacy value (as used in your constants.py for NA). In some ways this obviates constants.py and the change from NA to NA() (since you can recalculate NA = h.Avogadro_constant and others that depend on that whenever you have determined that h.nrnunit_use_legacy() has changed). I'm happy to leave things as they are but leave it up to you to decide whether to simplify your changes relative to this pull request. I should have said "almost single sourcing" as the modern units are also defined in share/lib/nrnunits.lib.in as well as src/oc/nrnunits_modern.h

@nrnhines nrnhines merged commit 6aff783 into master Oct 15, 2020
@nrnhines nrnhines deleted the dynam-units branch October 15, 2020 14:15

def switch_units(legacy):
try:
h.nrnunit_use_legacy(legacy)
Copy link
Member

Choose a reason for hiding this comment

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

@nrnhines : is this documented somewhere in user docuemtnaiton now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants