-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix filepath handling #208
base: master
Are you sure you want to change the base?
Fix filepath handling #208
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Philipp for these fixes! I think handling filenames consistently has high value for automation purposes and the next step in running codes. Happy to approve from my side :)
Could we actually make the gradient files not be hidden? I don't think it makes much sense to do so and I actually find it very frustrating. It makes the run result stateful, but then hides the state. About the writing to the folder where the input file is located, I agree that it is the best choice given that it doesn't break existing functionality. I'll work on a little fix for simsopt so simsopt copies the spec input file to the pwd, to avoid clobbering issues |
Nice, glad we are all on the same page :) Simsopt already has an optional argument to copy the files to pwd when creating the freeboundary default file, presumably because the hidden file handling was causing issues. mhd.Spec.default_freeboundary(copy_to_pwd=True) |
List of all hidden files (could we add this table of output files to the documentation? I added the missing ones to grp_output for now in the readme fix branch):
Do you @smiet think all of them should be "regular output files" then? I assume the reasoning was to limit how much the workspace is cluttered by each SPEC run. Also all hidden files are conditionally toggled on and off by some flags, while the regular output files are always generated by a SPEC run. Once again this would be a breaking change tho, and requires an update to the simsopt Spec wrapper, which accesses |
@abaillod, @jons-pf, @zhucaoxiang , what do you think about making these files visible? We should probably move this discussion to an issue, @abaillod can of you take care of that? (Or shoot down my suggestion?) For now the fix solves the issue and doesn't change existing functionality. Can you also trigger the tests and merge if all are successful? @missing-user thank you so much for your work. I'm going to be traveling in the coming week so I won't be super responsive, my apologies for that |
I would be in favour of making all produced files visible, because I have struggled myself in the early days with not being aware of them. One consideration to keep in mind in this regard is that users might have lots of files using the old naming scheme on disk (thinking mainly of @SRHudson, but potentially also others) - I would propose to make the writing use only the new scheme, but additionally (for now) keep the reading also backwards-compatible. |
Solves issue #206 related to running SPEC from a different working directory than the input file.
Currently writes both output files and hidden files to the same directory as the input file.
The most interesting change is in the first commit, where we define the filepaths for the output files
ext
and the filepath for the hidden fileshiddenext
(derivative, hessian, ...)