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

Moved the memory allocation for rxd reactions to register_rate. #2631

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

adamjhn
Copy link
Member

@adamjhn adamjhn commented Dec 7, 2023

Should resolve #2622

@adamjhn adamjhn added the rxd reaction-diffusion label Dec 7, 2023
@adamjhn adamjhn requested a review from ramcdougal December 7, 2023 13:30
Copy link

✔️ cffca52 -> Azure artifacts URL

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (638c30c) 66.30% compared to head (a93e7e3) 66.28%.

Files Patch % Lines
src/nrnpython/rxd.cpp 91.34% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2631      +/-   ##
==========================================
- Coverage   66.30%   66.28%   -0.02%     
==========================================
  Files         558      558              
  Lines      108424   108393      -31     
==========================================
- Hits        71887    71846      -41     
- Misses      36537    36547      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

✔️ e7f15e5 -> Azure artifacts URL

Copy link

✔️ 7f4c46b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@pramodk
Copy link
Member

pramodk commented Dec 12, 2023

@alkino: maybe you can help here to make Sonar check happy?

@alkino
Copy link
Member

alkino commented Dec 12, 2023

Except if we want to change everything to use smart pointers instead of this huge structure of mallocs. We should go with this bad sonar review.

@pramodk
Copy link
Member

pramodk commented Jan 7, 2024

Except if we want to change everything to use smart pointers instead of this huge structure of mallocs. We should go with this bad sonar review.

Ok.
Just for the record, I was wondering if SonarCloud will complaint for all future CIs but Nico clarified to me that Sonar will check only for modified code parts. So this is not the problem.

@bbpbuildbot

This comment has been minimized.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

Copy link

✔️ a93e7e3 -> Azure artifacts URL

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.

This is based on the recommendation from Robert in #2622, I think this is good to merge.

(As Nico pointed, the C-style memory allocations could be improved/avoided but that could be a separate change)

@adamjhn : if you have tested this, we can merge it ignoring sonar-cloud CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rxd reaction-diffusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rxd.cpp get_reaction_rates and solve_reaction possibly repeatedly mallocing.
4 participants