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

add options to override config file and tiles file #62

Merged
merged 5 commits into from
Sep 20, 2017
Merged

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Sep 20, 2017

This PR adds several features useful for small scale functional testing:

  • --config-file option for scripts enabling overriding the default configuration file
  • new config file tiles_file parameter to specify the DESI tiles file to use (requires desimodel >=0.9.0; defaults to desi-tiles.fits as before)
  • adds rules-layers.yaml with simplified "do layer N before layer N+1" rules for testing

Other changes along the way:

  • rename output directory default environment variable $DESISURVEY -> $DESISURVEY_OUTPUT since $DESISURVEY was already in use for installed packages at NERSC
  • prints ERROR message (but not FATAL crash) if any rules that don't cover any tiles
  • fixes 0.0/0.0 rules bug when there is only one tile being evaluated on a single tile such that hi==lo
  • tests work with desimodel test-0.9 branch of data instead of requiring trunk (on my laptop at least; we'll see if Travis agrees).

A companion update to surveysim will add the --config-file option there and update the tutorial instructions.

I suggest making tag 0.9.0 after this is merged (version file was already update but tag never made...)

@dkirkby
Copy link
Member

dkirkby commented Sep 20, 2017

I did make the 0.9.0 tag at the appropriate time, but forgot to push it (now fixed).

@dkirkby
Copy link
Member

dkirkby commented Sep 20, 2017

I see you edited changes.rst assuming that I didn't make the tag, so please update that.

@dkirkby
Copy link
Member

dkirkby commented Sep 20, 2017

Let's pull the docs at the head of rules.yaml and now rules-layers.yaml into a separate file (README.md?) so it isn't duplicated and easier to maintain.

slope = float(dec_order)
epsilon = float(hi>lo) #- used to avoid 0.0 / 0.0
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be float(lo>=hi) so epsilon is normally zero? In any case, epsilon=0 or 1 doesn't seem intuitive here. Perhaps simpler to just add a third if clause below?

if lo == hi:
  dec_priority[group_sel] = 1.
elif slope > 0:
  ...
else:
  ...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 38.163% when pulling 40f3432 on config into 9674ae9 on master.

@sbailey
Copy link
Contributor Author

sbailey commented Sep 20, 2017

  • Updated changes.rst with tag history
  • Fixed lo==hi logic (last minute "clarity" refactor before pushing broke... good catch)
  • Pulled rules.yaml and rules-layers.yaml doc header into separate doc/rules.rst doc.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 38.148% when pulling 4ef6ad7 on config into 9674ae9 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+18.6%) to 56.874% when pulling df848e8 on config into 9674ae9 on master.

@sbailey sbailey merged commit 982484d into master Sep 20, 2017
@sbailey
Copy link
Contributor Author

sbailey commented Sep 20, 2017

postfacto comment: the new unit tests run fast when using a desimodel test data subset, but are too slow when running on desimodel svn trunk data with all tiles. I will fix that now.

@weaverba137 weaverba137 deleted the config branch January 27, 2020 20:24
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