-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement reduction for DIPOL instrument #23
Conversation
8e9d1af
to
d5848e0
Compare
After adding the CAHA tests, 5d1ef9e failed in the CI but passed locally, reason apperars to be that if the reduction / access to the astrometry index files is done for the first time from the multi-process tests, httpdirfs crashes (fangfufu/httpdirfs#126 (comment)). As a workaround I have added a single-process tests for CAHA so that httpdirfs will cache the astrometry index files before the multiprocess test (5d3ef74). This test is not really needed except for the CI, so to save time it's marked to be skipped if not running in the CI. All tests local and CI are passing now (5d3ef74), also after refactoring the instruments (49e57a0). |
f34c1b4
to
7eb97d4
Compare
github limits the running time per month of actions in private repositories, it will reset on October 18th. Actions / CI will fail until then. |
you burnt the GH CI |
Yep 😢 Tests were taking long (~15min, with some failing tests running for almost 1h). Also too frequent pushes (I was using the repo as intermediate step to deploy). Let's hope that the actions cache alleviating the time needed and a change of deploy strategy is enough after 18th October to last the whole month with the 3000min that github allows us. Anyway, this is a problem for private repositories, but IOP4 should not stay private for long, it will help us and help others (fangfufu/httpdirfs#126, neuromorphicsystems/astrometry#6). |
necessary for standalone use of iop4api as a django app
Tests (local) are passing. DIPOL is implemented now, preliminary checks on the quality of the reduction also give good results. Full tests in CI are commented out because DIPOL test data is too big. But I think it's worth it to keep it in the test datasets. We can require tests to pass locally while we implement an alternative like a self-hosted runner in a secure way (we need to keep in mind that IOP4 repository will be public soon). If approved, I will squash and merge after cleaning the commit message. |
Commit ececc11 to see if GitHub limitations on its Actions runner can handle the DIPOL dataset. If it fails, I revert it. |
* using the target E,O method and a star image
It seems that the full tests can indeed pass in CI, so it must be still below the storage limit for the public runners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test passing, and DIPOL IOP4 results compatible with Vilppu pipeline. Let's merge this.
Work in progress
This PR implements the DIPOL-1 instrument for OSN-T090.
Now there will be two different instruments for OSN-T090, and in the in the future, there might be more telescopes with more than one instrument. Therefore I have decided to refactor instrument-specific functionality in instrument classes, in a analogous way as telescope-specific functionality.
Unlike the previous implemented instruments, DIPOL-1 uses CMOS camera instead of CCD, and needs dark frame correction to account for thermal noise, therefore this PR adds support for dark fame support and a MasterDark model.
I have improved tests coverage, now there are tests for CAHA-T220 CAFOS reduction and polarimetry is also tested, so we can be sure that we are not breaking functionality with this instrument refactor, and the new code to include DIPOL-1.
Many others changes have been included in this PR since its development has been long and many errors and bugs discovered -and some created- in the process.
This PR:
Other changes in this PR: