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

Fix DIPOL bugs, add quad tests, improve parallelization, other improvements #39

Merged
merged 33 commits into from
Nov 29, 2023

Conversation

juanep97
Copy link
Owner

@juanep97 juanep97 commented Nov 24, 2023

Solves #22 , #38, and some bugs introduced in #23.

Summary of changes:

  • Improves concurrency by setting sqlite DB to journal mode to WAL.
  • Implements parallel computing of relative polarimetry. Now, if max_concurrent_threads > 1, it reduces all polarimetry groups for each epoch in parallel. This reduces time a lot especially in nights with DIPOL data.
  • Fixes bugs in DIPOL reduction.
  • Adds tests for quad-matching algorithms.
  • Fixes bug in astrocalibration: mutable default function argument (8443b51)
  • Improves configuration of astrometry and choosing search radius.
  • Other minor improvements and bug fixes.

Concurrency has been improved greatly. Previously there was some isolated 'database errors' with ~10 cores, and many more with 12-14. Now I have tested with up to 16 cores and there are no errors, probably even more cores are fine too.

This is a nice point to create a release (#37).

@juanep97 juanep97 changed the title Dev Fix bugs related to recent DIPOL implementation, several improvements Nov 24, 2023
@rlopezcoto
Copy link
Collaborator

If I can suggest something for the future, to easier track changes, it is better to solve one bug at a time in single PRs

@juanep97
Copy link
Owner Author

juanep97 commented Nov 24, 2023

If I can suggest something for the future, to easier track changes, it is better to solve one bug at a time in single PRs

I know, sorry. The pace at which we have made changes lately to have a functional pipeline for the latest campaigns has forced us to work at the same time on many issues.

iop4lib/utils/__init__.py Outdated Show resolved Hide resolved
iop4lib/db/fitfilemodel.py Outdated Show resolved Hide resolved
@juanep97 juanep97 changed the title Fix bugs related to recent DIPOL implementation, several improvements Fix DIPOL bugs, add quad tests, improve parallelization Nov 27, 2023
@juanep97 juanep97 changed the title Fix DIPOL bugs, add quad tests, improve parallelization Fix DIPOL bugs, add quad tests, improve parallelization, other improvements Nov 27, 2023
the func modifies the value so subsuquent iterations had different start args

it was making some files fail in bulk reduction but not when processed alone

in general, it is not recommended to have mutable types as default args
now trying to use missing Instrument definitions will give error
Copy link
Collaborator

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I just left some minor comments. I think this PR should be merged soon

iop4lib/db/epoch.py Show resolved Hide resolved
iop4lib/instruments/dipol.py Outdated Show resolved Hide resolved
iop4lib/instruments/dipol.py Show resolved Hide resolved
iop4lib/instruments/dipol.py Show resolved Hide resolved
Copy link
Collaborator

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

LGTM

it has not been used in along time and it is not mantained

it is not really needed after the last improvements in parallelization
@juanep97 juanep97 merged commit 81e864c into main Nov 29, 2023
4 checks passed
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.

3 participants