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

Refactor src/gnu #1330

Closed
4 of 15 tasks
alexsavulescu opened this issue Jun 11, 2021 · 8 comments
Closed
4 of 15 tasks

Refactor src/gnu #1330

alexsavulescu opened this issue Jun 11, 2021 · 8 comments

Comments

@alexsavulescu
Copy link
Member

alexsavulescu commented Jun 11, 2021

Not familiar with all the code in gnu (except for the d_avec refactoring to std::vector)
A couple of ideas to brainstorm:

@nrnhines
Copy link
Member

I'd be delighted to see the gnu subdirectory disappear.

@alexsavulescu
Copy link
Member Author

Was discussing today about PRNGs, we were wondering if we shouldn't drop some unused number generators?

https://nrn.readthedocs.io/en/latest/python/programming/math/random.html?highlight=random#Random.ACG

As far as I understand , only Random123 and MCellRan4 are of interest wrt support for parallel simulations. Do we have any usecase for the rest? If so, I imagine we can use std implementations for most of them (readily available)

cc: @jamesgking

@ramcdougal
Copy link
Member

ramcdougal commented Jul 7, 2021

For what it's worth, MLCG and ACG do not appear to be used in any ModelDB entries.

@alexsavulescu alexsavulescu changed the title Refactor src/gnu ? Refactor src/gnu Aug 24, 2021
@alexsavulescu alexsavulescu added this to the Release v8.2 milestone Mar 28, 2022
@pramodk pramodk removed this from the Release v8.2 milestone May 24, 2022
@alexsavulescu alexsavulescu mentioned this issue Jun 28, 2022
19 tasks
@alkino
Copy link
Member

alkino commented Nov 19, 2024

Impossible. The results will now depends of the stdlib used (more or less gnu or clang). We will never be able to keep the tests working.

Quit.

@alkino alkino closed this as completed Nov 19, 2024
@sanjayankur31
Copy link
Contributor

Could you please clarify a little more what you mean? That results (simulation output like spike times?) will change with the version of stdlib in use?

@alkino
Copy link
Member

alkino commented Nov 19, 2024

We cannot modify distributions because if we do, results would be different depending of the standard lib used.
And more over it will modified all results.

@1uc
Copy link
Collaborator

1uc commented Nov 19, 2024

@sanjayankur31 The important word is the "Quit". After considerable amounts of time trying to rework the random number generation, and after painstakingly checking that the old and new code produces the (if it's not buggy) same distributions, he noticed that it's simply not possible to modernize this part because it would break many tests and also make the sequence of random numbers generated dependent on the things he mentioned. Therefore, it looks like he cancelled all outstanding efforts to modernize this part of the code.

None of the PRs that would have introduced the undesirable behavior mentioned were merged, IIUC.

@sanjayankur31
Copy link
Contributor

Thanks both, that makes sense. Thanks for looking into it @alkino---I had briefly looked into it initially (in #145) but given up quite early when I'd seen the scale of the task. You made a lot more progress and are certainly braver than me :)

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