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

Splitsteps #368

Merged
merged 18 commits into from May 27, 2021
Merged

Splitsteps #368

merged 18 commits into from May 27, 2021

Conversation

araichoor
Copy link
Contributor

This PR puts as arguments the steps to be run in fba_launch (dotile, dosky, dogfa, domtl, doscnd, dotoo, dofa, dozip, doqa, domove).
Previously, those steps (initially introduced to allow debugging) were hard-coded.
Now, those are controlled by two arguments, as suggested by @schlafly:
--steps : defaulting to all steps
--nosteps : defaulting to None
Only one can be set a time.

This does not change at all the fiber assignment, but offers more flexibility.
Noticely, in prevision of running fiberassign on-the-fly, this would allow to execute a first call generating fiberassign-TILEID.fits.gz; and a second call to make the QA png file would be done just after, but wouldn t delay the operations.
It would look like:

fba_launch --nosteps qa [...] # call to generate fiberassign-TILEID.fits.gz
fba_launch --steps qa --log-stdout [...] # call to generate the QA png file, redirecting to stdout to not overwrite the log file from the first call

To achieve that we moved the fiber assignment statistics report from the make_qa() function to the launch_onetile_fa() function, and disabled all log printing in make_qa().
We also allow to run e.g. with no gfa or no sky, which could be useful for testing.

@araichoor
Copy link
Contributor Author

@sbailey : in the perspective of fiberassign on-the-fly, we discussed with @schlafly some subtleties to move files around, required by this two-step approach.
This is because of the way the code is designed, as we prefer that the code does not work directly in the svn folder (i.e. we d prefer that args.outdir is not the svn folder).

A suggestion was to add an argument, e.g. --archivedir $FIBER_ASSIGN_DIR, which would copy the fiberassign-TILEID* files to the svn folder when those files are created.
Please let me know if you think I should implement this tomorrow, thanks!

@schlafly
Copy link
Contributor

schlafly commented May 27, 2021 via email

@araichoor
Copy link
Contributor Author

ok, noted for --archivedir.
I ve pushed two more small commit (one cosmetic, and one with safety checks on the --steps and --nosteps arguments).

@araichoor
Copy link
Contributor Author

@schlafly : this optional splitting of the steps raises few potential issues, for not-regular use.
e.g. if one sets --steps tile only, the code would crash at the "move" step, as I ve hard-coded the presence of the fiberassign-TILEID.fits.gz file.
the 3 commits above fix that; though the code will still crash if some files expected by a requested step is not present, but that is what we want, I think.

@schlafly
Copy link
Contributor

I agree that it's fine for something to crash if you ask for a step when the expected outputs for that step aren't available.

@sbailey
Copy link
Contributor

sbailey commented May 27, 2021

I tested that the code produces the same answer as before if not using the --steps / --nosteps options, and that it works as expected with the primary use case of --nosteps qa (i.e. produces other outputs the same, just not the qa png file).

I'll merge now, and other refinements can come in additional PRs later.

@sbailey sbailey merged commit cdbe4fd into master May 27, 2021
@sbailey sbailey deleted the splitsteps branch May 27, 2021 21:00
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