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

Move high-level code into the package #188

Merged
merged 7 commits into from Mar 6, 2019
Merged

Move high-level code into the package #188

merged 7 commits into from Mar 6, 2019

Conversation

tskisner
Copy link
Member

@tskisner tskisner commented Mar 5, 2019

This work moves the internals of the entry point scripts into the package. The functions for parsing arguments and running the code are now in the fiberassign.run sub-module and are added to the API reference in the documentation. Other changes:

  • The new option to fba_run (--mask_column, default "DESI_TARGET") can be used to override the column in the FITS files containing the target bits.

  • A new option to fba_run (--by_tile) does the outer loop over tiles and fully assigns each tile in sequence before moving on to the next. @sbailey , can you verify that fiberassign.run.run_survey_bytile() essentially does the looping in the way your modified version of fba_run was working?

This work should address #181, since now it is easier to run the assignment interactively for commissioning and also to override the FITS column used for mask operations.

@tskisner tskisner requested review from sbailey and Srheft March 5, 2019 20:14
Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

+1 for reorganizing this. Two requests inline (one algorithmic, one structural).

tile_id, tile_id)
asgn.assign_force(TARGET_TYPE_SKY, args.sky_per_petal,
tile_id, tile_id)

Copy link
Contributor

Choose a reason for hiding this comment

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

After doing these, we should have a final assign_unused for both standards and sky to fill in any remaining unused fibers since the previous calls max out at args.standards_per_petal and args.sky_per_petal. i.e.

#- I think this first one is a no-op, but would be good to confirm
### asgn.assign_unused(TARGET_TYPE_SCIENCE, -1, "POS", tile_id, tile_id)
asgn.assign_unused(TARGET_TYPE_STANDARD, -1, "POS", tile_id, tile_id)
asgn.assign_unused(TARGET_TYPE_SKY, -1, "POS", tile_id, tile_id)

With this addition, this is equivalent to what I tested before for my per-tile serial assignments tests.

with open(args.qafile, "r") as f:
qadata = json.load(f)
plot_qa(qadata, args.outroot, labels=args.labels)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

We're at almost a 1000 lines for this file and it is mixing the command line parsing and run functions for multiple different scripts separated by hundreds of lines of code for the pieces that go together. I'd prefer to organize these like desispec et al, with desispec/scripts/ and one module file per script file.

@tskisner
Copy link
Member Author

tskisner commented Mar 5, 2019

Ok, will make those changes and the single line of code to go from 5 -> 6 digits for the tile ID in the output files.

@Srheft
Copy link
Contributor

Srheft commented Mar 6, 2019

@tskisner : having trouble with correct use of --mask_column

  • Using the branch driver, my example tile file, MTL and SKY files are posted here.

  • The MTL file has a column called SV1_DESI_TARGET with target bits for SV that I aim to replace with the default DESI_TARGET via the new option --mask_column.

  • My runing command was: fba_run --targets ./MTL_DEEP2_1.fits ./33-40--3-3-skies-dr7.1-0.27.0.fits --footprint ./tiles_18pRosette_DEEP2-1.fits --mask_column SV1_DESI_TARGET --by_tile --sky_per_petal 80 --standards_per_petal 20 --dir ./FAout_deep1 --write_all_targets --overwrite

  • targets.py complains about the field name:
    fiberassign/targets.py", line 187, in append_target_table tgdata[typecol], sciencemask=sciencemask, stdmask=stdmask, ValueError: no field of name SV1_DESI_TARGET

…st. Still debugging why that test fails, and then will return to the mask_column issues.
@sbailey
Copy link
Contributor

sbailey commented Mar 6, 2019

Structurally looks good; thanks. Please resolve Sarah's test problems and the unittest failures (they appear to be real, not just failures of Travis itself) before merging.

@tskisner
Copy link
Member Author

tskisner commented Mar 6, 2019

The known problems are fixed, investigating the test dataset above now.

@tskisner
Copy link
Member Author

tskisner commented Mar 6, 2019

The input data files are missing in the link above, so I cannot test. I can coordinate testing with @Srheft offline. I would like to merge this branch since I am ready to open a PR that fixes #175, and which is based on code in this PR. I'll open a separate issue if needed for fixing any problems with the --mask_column option.

@tskisner tskisner merged commit deac69c into master Mar 6, 2019
@tskisner tskisner deleted the driver branch March 6, 2019 22:51
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

3 participants