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

extract fiberassign relevant information from surveysim #54

Closed
wants to merge 1 commit into from

Conversation

forero
Copy link
Member

@forero forero commented Oct 24, 2017

Added a simple script that extracts the information described in #53

@forero forero requested a review from dkirkby October 24, 2017 05:24
@forero
Copy link
Member Author

forero commented Oct 30, 2017

@dkirkby Do you have any comments about this script?

@dkirkby
Copy link
Member

dkirkby commented Oct 30, 2017

This looks like it probably meets your needs but has several issues if you want to add it to surveysim:

  • You are not following the recommended pattern for bin scripts, where most of the functionality lives in a separate scripts module. See bin/surveysim and surveysim.scripts.surveysim for example.
  • You are partially reimplementing utility code already provided in desisurvey.config and desisurvey.utils, which makes your code harder to maintain (due to duplication) and more fragile (since you hardcode assumptions that are handled more generally by the existing utilities).

Taking a step back, your code reads progress.fits and exposures.fits and writes:

  • exposures_bright|dark.fits
  • fiberassign_dates_dark|bright.txt

The latter file only contains dates (YYYY-MM-DD) without identifying which tiles should have FA run on each date. Do you actually need this info in files, or just in arrays in memory? How do you use the fiberassign_dates_*.txt files? Since the exposures table is obtained directly from the progress table, do you really want to read these files separately (i.e., to have the flexibility that they are non self consistent) or would it be better to just read progress.fits and derive the exposures table? Finally, your use of "dark" to include "gray" here is potentially confusing.

@forero
Copy link
Member Author

forero commented Oct 31, 2017

Thanks, @dkirkby . This is useful.

Answering your questions.

Do you actually need this info in files, or just in arrays in memory? How do you use the fiberassign_dates_*.txt files?

Right now I read the files when I run fiberassign from quicksurvey.

do you really want to read these files separately (i.e., to have the flexibility that they are non self consistent) or would it be better to just read progress.fits and derive the exposures table?

I want to have the freedom to change fiberassign_dates independent from the exposures table. I want to have that flexibility to run fiberassign on different dates.

Finally, your use of "dark" to include "gray" here is potentially confusing.

I need to either include gray into bright or gray into dark because I am keeping separate quicksurvey runs for dark and bright. Right now I cannot fit into memory the whole set of targets.

I will replace some of the code to use functions in desisurvey. Then I will implement this in a function somewhere else. Where do you think it would be the best place for such a function? desisurvey, surveysim? desisim?

@forero
Copy link
Member Author

forero commented Oct 31, 2017

I am closing this PR in the meantime.

@forero forero closed this Oct 31, 2017
@weaverba137 weaverba137 deleted the fiberassigndata branch December 20, 2019 18:12
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.

None yet

2 participants