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

code to support newtiles, and S vs. R #135

Merged
merged 10 commits into from
Feb 28, 2020
Merged

code to support newtiles, and S vs. R #135

merged 10 commits into from
Feb 28, 2020

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Feb 27, 2020

This PR provides code to match @schlafly's "new" tiling pattern in svn desimodel/branches/newtiles . That branch of the desimodel data expanded the format of the tiles file which broke some of the desimodel code (e.g. multi-dimensional columns), so this adapts to that. I also updated the tests to support both current trunk and the newtiles branch.

Another change that came along for the ride is splitting out the S vs. R vs. Z vs. N data from DESI-0530 into a separate file (in the newtiles branch) instead of adding S (arclength) as a new column in the platescale.txt file (trunk). The code here is compatible with either, but paves the way to update platescale.txt with the as-built model from DESI-4037 (issue #134) without breaking fiberassign.

@tskisner please review for fiberassign impacts.

@schlafly ok?

This PR replaces PR #95 that I just closed.

@schlafly
Copy link
Contributor

Looks good to me!

@sbailey
Copy link
Contributor Author

sbailey commented Feb 27, 2020

Hmm. I apparently had an old copy of the newtiles branch while testing, and the latest version of that data branch fails. I'm chasing those now...

@sbailey
Copy link
Contributor Author

sbailey commented Feb 27, 2020

Arg. UPPERCASE vs. lowercase column names. I just updated desi-tiles.fits and .ecsv in the newtiles branch to use UPPERCASE columns to match the datamodel of trunk (plus extra columns...).

@schlafly
Copy link
Contributor

Sorry. It looks like this update to SVN lost the metadata in the ECSV file. I can further update that later, though, so let's just press on...

@sbailey
Copy link
Contributor Author

sbailey commented Feb 27, 2020

Ah, sorry about the metadata. I updated ecsv by re-writing a subset of the fits columns as ecsv, rather than reading ecsv, updating, and rewriting the ecsv itself. But yes, let's press on... :)

Will wait for confirmation from @tskisner about fiberassign impact before merging.

@tskisner
Copy link
Member

Looks good to me

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.

4 participants