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

remove_avg_com_motion requires path_to_waveform_h5 option #68

Open
markscheel opened this issue Apr 22, 2022 · 1 comment
Open

remove_avg_com_motion requires path_to_waveform_h5 option #68

markscheel opened this issue Apr 22, 2022 · 1 comment

Comments

@markscheel
Copy link
Contributor

remove_avg_com_motion makes you provide path_to_waveform_h5 even if you tell it not to read that file (by passing in a waveform object w_m) and even if that file doesn't exist. That actual h5 file doesn't need to exist, but path_to_waveform_h5 must have the form 'something.h5/group' or '/full/path/something.h5/group', which it will always parse and it will throw an error if it cannot parse, because it uses the /full/path and group pieces later on in the code. If you don't provide path_to_horizons_h5 or path_to_matter_h5 arguments, then it will look for Horizons.h5 and Matter.h5 inside '/full/path' (or inside cwd if the given path begins with 'something.h5'). Also, both '/full/path' and 'group' will be used for output of figures and filenames of figure files if plotting is turned on.

Note that if your Horizons.h5 happens to be in cwd, then the default value of path_to_waveform_h5 will work correctly if you don't specify path_to_horizons_h5. (but if you turn plotting on, the plot filename will contain 'Extrapolated_N2' which might be misleading).

It would be less confusing if the options were done differently.

Perhaps path_to_horizons_h5 could be the required option, and then internally the variable directory would come from path_to_horizons_h5. Then path_to_waveform_h5 could be truly optional and have a default of None. The internal variable subdir could have its own option (which the user would be required to specify if path_to_waveform_h5 is None and if plotting is turned on).

@markscheel
Copy link
Contributor Author

markscheel commented Apr 22, 2022

I came across this because I used the new w_m option of remove_avg_com_motion and I thought the 'obvious' thing to do was to pass it path_to_waveform_h5=None since I didn't have a waveform h5 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

No branches or pull requests

1 participant