-
Notifications
You must be signed in to change notification settings - Fork 4
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 tiles #95
Conversation
Have you tested the GRAY pass re-numbering with desisurvey and surveysim? This might not be the best moment for downtime on those packages while this is sorted out. |
I also wanted to ask about whether / how the footprint changed along the edges with this new tile file. Will we need all new mock catalogs or is the new footprint inclusive of the old one? Apologies for jumping in with this question on this particular thread, but that could be another downstream wrinkle of this new tile file. |
desisurvey/surveysim: tests pass on my laptop but fail at NERSC due to a large negative MJD generated somewhere in the chain. The desisurvey rules files have not been updated to account for the new layer numbering. For the purposes of survey margin studies you could continue to use the latest desimodel tag. Edge effects: the basic footprint is the same, but the new dithering has a much more scalloped pattern along the edges and I'm sure there are some healpix that come and go from new to old. We hadn't considered the impact on whether new mocks would need to be generated. Worst case is that we back out to the old tile file as default while continuing to test the impact of the new one. Since we will need to do at least one more update to the footprint to match the final imaging, we should consider each of these cases for why re-defining the tiles isn't just a trivial replacement in the hopes that it will be easier in the next round. |
…rect behavior when tiles is None
@sbailey: this unit test failure uncovered a (mild) bug. When passed a set of tiles, the I think I've fixed this, and, as a bonus, I made the test more realistic as the calculation is quicker once the |
Thanks @geordie666 we're in a state where merging this PR would fix desimodel to work with the new tiles, but we still have outstanding questions about the impact on desisurvey and mocks so I'm not going to merge just before going into the weekend. |
This PR is on hold while we get the mocks ready to use the new tiles. In the meantime, the new tiling file is in a desimodel svn branch "newtiles" and I've updated the travis tests of this PR to test against that instead of master. |
It's the case that this new tile file made its way into If so, this is causing issues with Or do I need my own local checkout of $DESIMODEL?
|
@moustakas something else is wrong. I restored the previous tile file into desimodel svn trunk last week, and on the code side PR #95 hasn't been merged, so master should be ok with the old tile file. What is the symptom that made you think we still had the new tile file in master/trunk?
|
@sbailey, is this ever going to be merged? Also, is there any reason to keep the already-merged branch |
Some version of this branch will be needed to support the data model changes of the new tile file in the SVN newtiles branch. I do want to do that, but I'm trying to get everything else back into a working state and then introduce the new tiles as a single isolated change instead of throwing it into the broken mix too. I just deleted the state gfa branch. |
Thank you. Sounds like I can ignore this PR for the purposes of a 19.9 release then. |
Can we resurrect this issue? In particular, if this new tile file has knowledge of the final imaging footprint (as defined by DR8) that would be very useful for DR9 prep. @sbailey @djschlegel |
Could someone please summarize the remaining issues that need to be addressed before this can be merged? This branch is sufficiently old that it may need to be rebased on master (that's something I can do). |
closing this stale PR. Will open a new PR that supersedes this. |
Do we need to keep the |
We have a new desi-tiles.fits file with a better dithering, but it broke code and tests for several reasons:
This PR mostly fixes the code and tests to work with this new tile file, but I'm still having trouble with
desimodel.test.test_footprint.test_partial_pixels
, which sometimes passes and sometimes fails. The original test was already quite loose in testing the consistency between high and low resolution pixel weighting. @geordie666 could you take a look and help with that?