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

Optimize masks #984

Merged
merged 14 commits into from
May 20, 2023
Merged

Optimize masks #984

merged 14 commits into from
May 20, 2023

Conversation

iprafols
Copy link
Collaborator

@iprafols iprafols commented Mar 20, 2023

Currently, masks take too long (in some cases more than the actual continuum fitting process). This PR intends to fix this

This PR is still on-going. Some things needed before this is ready to review

  • Add masks tests
  • Make sure the numba functions run properly
  • Estimate time improvement

@iprafols iprafols marked this pull request as draft March 20, 2023 15:16
@iprafols iprafols marked this pull request as ready for review April 4, 2023 10:58
@iprafols
Copy link
Collaborator Author

iprafols commented Apr 4, 2023

I have moved functions related to BAL and DLA masking outside the classes and numbaised them. The code seems to run fine (at least passes the test, including the new mask tests from PR #989). I also restructured the BAL masking code so that the actual computation is done inside the function apply_mask instead of it being computed at read-time. This will give us higher fidelity in the time logs and should also improve the speed as calls to apply_mask are parellelized

@iprafols
Copy link
Collaborator Author

iprafols commented Apr 4, 2023

I still need to see how much faster the code runs. I was planning to run some tests on fugu data but @alxogm maybe you can also run on Iron to see if this solves your issue or if we still need to speed up the code

@iprafols
Copy link
Collaborator Author

iprafols commented Apr 7, 2023

I checked the times from the speed tests here: /global/cfs/projectdirs/desi/users/iprafols/lya_working_group/picca/speed_tests

Timing for the new branch

[INFO]: Time spent reading masks: 3.26883602142334
[INFO]: Time spent applying masks: 554.9469048976898
[INFO]: Total time elapsed: 1490.8811736106873

Timing for the master branch

[INFO]: Time spent reading masks: 76.29190325737
[INFO]: Time spent applying masks: 1470.6445696353912
[INFO]: Total time elapsed: 2192.240534067154

@iprafols
Copy link
Collaborator Author

iprafols commented Apr 9, 2023

I checked the final deltas in an end-to-end Fugu run and there are some small changes in some of the deltas. I tried the branch running without DLA masking and the results are equal to the master branch. I also tried the branch without BAL masking and the results are slightly different. This seems to indicate that the difference comes from changes in the DLA masking.

@iprafols
Copy link
Collaborator Author

iprafols commented Apr 9, 2023

not sure why the changes are not picked up by the tests

@Waelthus
Copy link
Contributor

I guess we should wait for the DLA/BAL testing branch to see which check is failing in here...

@iprafols
Copy link
Collaborator Author

I have re-run this branch on fugu data and checked the impact on the correlation function, as well as the delta attributes:

Here are the run time statistics. Both master and this branch have been run 3 times
Captura de pantalla 2023-05-16 a les 11 37 53

Comparison of variance statistics:
download

Comparison of delta stack:
download-1

Comparison of quasar continuum:
download-2

Comparison of metadata:
download-3

Comparison of lya autocorrelation:
download-4
download-5

Given all this, I think it is safe to merge this branch

@iprafols
Copy link
Collaborator Author

@Waelthus @andreicuceu can you review this PR?

@Waelthus
Copy link
Contributor

agreed, order 1e-8 changes of the correlation function should not be something to be scared of. And having 30% faster calculations definitely is nice!

@iprafols
Copy link
Collaborator Author

@Waelthus can you approve the PR then?

@iprafols iprafols merged commit 53e1c99 into master May 20, 2023
@iprafols iprafols deleted the optimize_masks branch May 20, 2023 09:51
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.

2 participants