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

Change the function validLib to improve the code efficiency. #255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

olivierilbert
Copy link
Collaborator

Change the function validLib in onesource.cpp. Rather than providing fullLib as an argument, I passed the redshift from the library. The price for manipulating the large vector of complex objects (fullLib) was slowing down the code. With this modification, we gain almost a factor 3 in speed when zfix=yes. Results are exactly the same. I think that such change could be done in few other functions.

@olivierilbert olivierilbert linked an issue Dec 10, 2024 that may be closed by this pull request
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.99%. Comparing base (1164c3c) to head (c0dce65).

Files with missing lines Patch % Lines
src/lib/onesource.cpp 50.00% 2 Missing ⚠️
src/lib/photoz_lib.cpp 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   71.93%   71.99%   +0.06%     
==========================================
  Files          50       50              
  Lines        6477     6478       +1     
  Branches      940      940              
==========================================
+ Hits         4659     4664       +5     
+ Misses       1818     1814       -4     

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

Copy link
Member

@johannct johannct left a comment

Choose a reason for hiding this comment

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

LGTM but we can push a bit forward :

  • you mention a few other places where this could be done. It is the right place to do it
  • zfix arg in validLib is not really sound. In photoz_lib zfix should be tested and validLib called only when zfix==true. Note that a vector with the first N integers is obtained with std::iota :
    #include <numeric> std::vector<int> vec(fullLib.size()); // fills the vector from 1 to N std::iota(vec.begin(), vec.end(), 0);
  • a unit test is easy to build and should be done
  • should we take this opportunity to write a notebook that shows the usefulness of running with zfix==yes?

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

Successfully merging this pull request may close these issues.

zfix=yes too slow
2 participants