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

overwrite issues with desisim/master/bin/wrap-fastframe #493

Open
michaelJwilson opened this issue May 29, 2019 · 6 comments
Open

overwrite issues with desisim/master/bin/wrap-fastframe #493

michaelJwilson opened this issue May 29, 2019 · 6 comments
Assignees

Comments

@michaelJwilson
Copy link

michaelJwilson commented May 29, 2019

Lines 83/84 ignore the relevance of args.clobber. If "to clobber", all simspec files should be appended, not just those that do not exist.

Similarly, logfiles should be placed in the dir. referred to by --outdir rather than the dir. of original simspec. Otherwise, e.g. there can be file permission issues when running on a preproduced set of simspec files.

@michaelJwilson
Copy link
Author

michaelJwilson commented May 30, 2019

Ok, I think these changes are done:

i) add args.clobber to generation of simspec list.
ii) place logfiles in outdir if provided, otherwise in original simspec as previously. Logfiles inherit /night/exposure/ structure of simspec files in both cases.
iii) --cframe run off the sv branch (#494) to use appropriate bits.

Largely goes through fine. Output is here:

/global/cscratch1/sd/mjwilson/svdc-summer2019/spectro/fframe/v1/

Not sure how worrying
WARNING: Tried to get polar motions for times after IERS data is valid. Defaulting to polar motion from the 50-yr mean for those. This may affect precision at the 10s of arcsec level [astropy.coordinates.builtin_frames.utils]

should be.

My problem is that I already have a fork of the desisim repo. with changes to master for BEAST calculations. So I can't submit a pull request, given I can't change the remote directory for this new clone of desisim to point to my github. Any suggestions?

The code itself is here:
/project/projectdirs/desi/users/mjwilson/repos/desisim/bin/wrap-fastframe

@michaelJwilson
Copy link
Author

caveat: --clobber is passed to fastframe by the wrapper, which doesn't parse --clobber as an argument. Ultimately, desispec.io.write_frame assumes overwrite=True, and uses os.rename to move a .tmp to the destination, both irrespective of args.clobber.

@sbailey
Copy link
Contributor

sbailey commented May 30, 2019

Short version: thanks, I'll incorporate your changes into my sv branch in PR #494 .

Long version:

Lesson learned on always working in a branch instead of directly in master. From the DESI wiki UsingGit page:

If you made changes to master and you want to retroactively turn that into a branch for further development and/or sending a pull request

#- Go ahead and commit whatever you have
git commit -am "comment about what changed"

#- Create a new branch starting from this point
git branch newbranchname

#- Reset your local master to whatever github has
git reset --hard origin/master

#- checkout your branch and keep working
git checkout newbranchname

IERS warnings are harmless, having to do with not being able to predict the Earths axis wobble (nutation) years into the future.

@michaelJwilson
Copy link
Author

ok, great. Looks like it will work. Note the additional clobber issue for fastframe. It wasn't totally obvious what'd you want in that case.

@sbailey
Copy link
Contributor

sbailey commented May 30, 2019

I think what you did with wrap-fastframe clobber is correct: since fastframe itself will clobber if called, its up to wrap-fastframe whether to call it, based upon --clobber and the pre-existence of files (if not clobbering). Thanks for this.

@michaelJwilson
Copy link
Author

Need to port the "if not done, do" logic to the output dir calls. I have the fix.

Any objection to providing expose types as an argument? Defaulting to flat,science.

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

No branches or pull requests

2 participants