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

Check if input tile files exist before looking in data folder. #98

Merged
merged 5 commits into from
Feb 11, 2019

Conversation

tskisner
Copy link
Member

@tskisner tskisner commented Feb 6, 2019

This addresses the problem in #97

@tskisner tskisner requested a review from sbailey February 6, 2019 20:51
@sbailey
Copy link
Contributor

sbailey commented Feb 6, 2019

I commented on the issue before seeing this PR, so let's continue the discussion on issue #97 to converge on the behavior we want, and then update this PR as needed to match.

@weaverba137
Copy link
Member

I have a concern: Is this change going to apply to every file in $DESIMODEL, or only the tile file(s)? If the former, we should document the search procedure as it applies to every file, perhaps in a high-level section in the doc directory. If the latter, we should document why the search method applies only to the tile files.

@weaverba137
Copy link
Member

And speaking of tile files, what's up with #95?

@tskisner
Copy link
Member Author

Do not merge this yet- I am checking one other strange corner case.

@sbailey
Copy link
Contributor

sbailey commented Feb 11, 2019

Thanks for the update @tskisner. Quoting you from issue #97:

  • Check if the local file exists
  • Check if the file in $DESIMODEL/data exits
  • If neither exists, raise an exception
  • If one exists, read it
  • If both exist, print a warning and choose the one from $DESIMODEL

@weaverba137 this PR only applies this logic to the tiles file, but I think it is a reasonable approach for any case where an I/O function allows the user to override an input file that is otherwise in $DESIMODEL/data. The basic logic could be moved into a desimodel.io utility function and then updated throughout, but I'm not going to block this PR for that.

The new tiles file in #95 is waiting in the wings for final testing with desisurvey and mocks. Updates have been made for consistency with that tile file, but I don't think anyone has found the time to do the final vetting that all pieces work together before putting the new tiles file in place.

@sbailey sbailey merged commit b3178e7 into master Feb 11, 2019
@sbailey sbailey deleted the tilepath branch February 11, 2019 18:00
@tskisner
Copy link
Member Author

Looks like I did not get my message here fast enough before merging. I'll try to fix the case I saw and push directly if it is a small fix.

@tskisner
Copy link
Member Author

OK, nevermind. This branch was fine. The problem I saw was actually in fiberassign which was duplicating the behavior of this new code (i.e. looking up the default footprint file). So it was passing in the full path to the default footprint file and triggering a warning in this new desimodel code (since that full path pointed to a file with the same name as the default one- because the are the same file).

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