Remove STD_WD from the per-petal STD counting

[araichoor]

This PR addresses issue https://github.com/desihub/fiberassign/issues/410. It discards STD_WD from standard counting in targets.default_main_stdmask(). @sbailey: the fix is simple, however I d like to have advice on the choices I made.

To ensure backward-compatibility, I propagate the rundate argument down to that function (defaulting to None), and add a switch based on a cutoff rundate. I have currently set this cutoff date to 2021-10-01T19:00:00+00:00: we would have to merge this PR and make a fiberassign tag before tomorrow night, or change that cutoff date in the code, if merging+tagging later. Also, I have made the default behavior to discard STD_WD (i.e. STD_WD would be included only if a rundate argument is provided, and if it is before the cutoff date).

In preparation of future similar events, I ve added a get_date_cutoff() function; the idea is that it would centralize the cutoff dates (rundate, mtltime, etc) for any future similar changes. I ve added that function in fba_launch_io.py: however proceeding this way adds a dependency to fba_launch_io.py in targets.py; I had to move around few imports in fba_launch_io.py to avoid some circular imports issue, as fba_launch_io.py imports some modules, which import targets.py. That is a possible significant drawback: if advised to do so, I can remove that get_date_cutoff() function, thus removing any dependence to fba_launch_io.py in targets.py; the cutoff date would then just appear in targets.default_main_stdmask().

Lastly, I notice that default_main_stdmask() function is also used in fiberassign/qa.py and indirectly in fiberassign/vis.py; apparently that is used in the test suite. I am not familiar with those scripts, I have not modified those files for now in this PR; I just checked that python setup.py test still runs; I think that the default rundate=None argument just propagates through the functions. Same, if advised to do so, I could have a closer look to that.

[araichoor]

in case useful, details about the circular imports issue I encountered (@schlafly):

• I added in fiberassign.targets.py:

  from fiberassign.fba_launch_io import assert_isoformat_utc, get_date_cutoff

• however, fiberassign.fba_launch_io.py has (at the top of the script in the master version) imports from fiberassign.scripts.assign.py and fiberassign.assign.py:

  from fiberassign.scripts.assign import parse_assign, run_assign_full
  from fiberassign.assign import merge_results, minimal_target_columns

• and fiberassign.scripts.assign.py and fiberassign.assign.py have imports to fiberassign.targets.py; e.g. in fiberassign.assign.py:

  from .targets import (TARGET_TYPE_SCIENCE, TARGET_TYPE_SKY, TARGET_TYPE_SUPPSKY,
                        TARGET_TYPE_STANDARD, TARGET_TYPE_SAFE, desi_target_type,
                        default_target_masks, default_survey_target_masks)

  I solved that by moving the fba_launch_io imports from scripts.assign and assign inside the fba_launch_io.create_mtl () and fba_launch_io.launch_onetile_fa() functions, where those are actually used.

[schlafly]

I played with this for a second. It looks like you could get around this if you chose by replacing the from x import y calls with basic "import x" followed by calls to "x.y(...)". I had not appreciated this before, but apparently from x import y requires running module x, but just "import x" has some partial initialization mode where it just figures out the names of everything but doesn't try to run them. https://dev.to/deepjyoti30/why-you-should-keep-away-from-cyclic-imports-in-python-1n75 But I think your solution is fine as well.

[araichoor]

thanks a lot for having looked into that, and for the tip about importing only the parent package. I ve tried to implement the approach you mention (so that the imports all remain at the top of the scripts, where the potential circularity issue is mentioned, I find this way more explicit), but the "Checks" on the commit failed, because of some consequences in other scripts... (see https://github.com/desihub/fiberassign/pull/415/commits/8572cdb8ead16ff7b242d256d26e8312424efa69).

so I reverted back to my original coding, which seems to work (though less nice than your suggestion).

[schlafly]

Ugh, sorry for the extra work, I probably only went as far as testing that the main fba_launch stuff imported without issue, but did not run the full test suite. Sorry.

[araichoor]

no worries, one could not know (I didn t consider that either). I guess the lesson is that those circular imports issues are quite nasty... my current coding seems to work, but I suspect it is kind of fragile..

maybe a better approach, instead of defining this get_date_cutoff() function, would be to create a py/fiberassign/data/date_cutoff.yaml file with listing that information? that would remove this circular import thing, while keeping this functionality to store at one place the various relevant cutoff dates?

I can try that tomorrow.

[araichoor]

pinging @sbailey about the idea of creating a cutoff-date.yaml file in (a newly created) py/fiberassign/data folder, and use that to store at one place the dates used in fiberassign for "if date > cutoff_date then do this else do that". that would prevent those possible circular import issues, and prepare a structure to store other cutoff dates; there currently is only one date, but I feel it is likely to happen again during the survey.

It could look like this:

rundate:
    std_wd: "2021-10-01T19:00:00+00:00" # std_wd not counted in per-petal-std after that rundate

And be used as follows:

fn = resource_filename("fiberassign", "data/cutoff-dates.yaml")
f = open(fn, "r")
config = yaml.safe_load(f)
f.close()
rundate_cutoff = config["rundate"]["std_wd"]

Would that make sense? If we go that way, I could also put in that .yaml file various dates used for "bug-fix" in rerunning sv3 in fba_rerun_io.py, e.g.: https://github.com/desihub/fiberassign/blob/ac6e935614cad8c888f2bd5b93000eb955185292/py/fiberassign/fba_rerun_io.py#L161-L327 That would be for a subsequent PR, of course. @schlafly suggested doing something like that at some point, if I remember correctly.

[sbailey]

@araichoor that sounds good. I like the idea of collecting all of these cutoff dates into a single place with comments explaining what they are for.

Let's wrap the access in an function like

def get_rundate_cutoff(cutoff_name):
    """document..."""
    fn = resource_filename("fiberassign", "data/cutoff-dates.yaml")
    with open(fn, "r") as f:
        config = yaml.safe_load(f)
    rundate_cutoff = config["rundate"][cutoff_name]
    return rundate_cutoff

So that the N>1 places that need a cutoff date just need to call a function instead of putting together nearly identical resource_filename, yaml.safe_load, etc. logic.

[araichoor]

ok great, I ll do that, thanks for the prompt answer!

and, yes, 100% agreed about making a function to read that file. one additional remark here: I think I ll create a new file (py/fiberassign/util.py?), where I would store such functions. so far I ve put ~everything in fba_launch_io.py, but, because of this circular import stuff, such a function get_rundate_cutoff() cannot live in fba_launch_io.py.

and I could then move from fba_launch_io.py to util.py some functions which are general. as e.g.: https://github.com/desihub/fiberassign/blob/ac6e935614cad8c888f2bd5b93000eb955185292/py/fiberassign/fba_launch_io.py#L75-L89 https://github.com/desihub/fiberassign/blob/ac6e935614cad8c888f2bd5b93000eb955185292/py/fiberassign/fba_launch_io.py#L152-L175 https://github.com/desihub/fiberassign/blob/ac6e935614cad8c888f2bd5b93000eb955185292/py/fiberassign/fba_launch_io.py#L178-L201

[schlafly]

+1 for moving code that isn't tightly coupled to fba_launch into something like https://github.com/desihub/fiberassign/blob/master/py/fiberassign/utils.py Let's not add a new file util.py in addition to the existing utils.py, though!

[araichoor]

ok, re-perfect; and thanks for pointing this utils.py file, I just never noticed its existence.. I ll start to work on a new, separate, PR dedicated to this structure-reorganization (moving stuff to utils.py, create a .yaml file).

[araichoor]

Tests are failing because they apparently cannot find the newly added data/cutoff-dates.yaml file. I apologize, I may need help here (@sbailey ?). In case it's useful:

• in the code, that file is searched with resource_filename("fiberassign", "data/cutoff-dates.yaml");
• I've simply added that file with git add, then committed with git commit.

[araichoor]

I thought I'd be smart with mimicking what is e.g. in desitarget with adding in setup.py setup_keywords['package_data'] = {'fiberassign': ['data/*',],}. But then the checks were not running, with only displaying "Waiting for checks information...". So, I've reverted that commit; however the checks are not running either now. I hope I have not break anything, maybe setup.py is file needing special treatment...