Moving em_progenitors.py and ns_sequence data from tmpltbank to new neutron_stars package #3937
Conversation
There was a problem hiding this comment.
Hi @SamuelH-97, thanks for this PR. A few initial comments above. I'll take a deeper look soon.
a-r-williamson
force-pushed
the
EMbright_branch
branch
from
947df63 to
d6c18df
Compare
a-r-williamson
force-pushed
the
EMbright_branch
branch
from
39f0d9a to
5e16322
Compare
…ant NS functions moved to neutron_stars. Calls to these functions fixed to reflect new locations
…ort issues (currently unresolved)
…kage. Edited how said functions are imported elsewhere where necessary. Added neutron_stars package path to setup file
…imports and old code comments from during development
…ror with clear error messages. Co-authored-by: Andrew R. Williamson <[email protected]>
|
I took another deep look through, and left some trivial comments and a more substantial one. I think those will be my last comments. |
Co-authored-by: Tito Dal Canton <[email protected]>
…nd outputs in a number of functions
Co-authored-by: Tito Dal Canton <[email protected]>
Co-authored-by: Tito Dal Canton <[email protected]>
There was a problem hiding this comment.
I only have 1 question to raise for @SamuelH-97. In turn it raises a check for @cdcapano .
|
@spxiwh recommends moving the large pickle file to the pycbc-config repository. I agree the file is a bit large to keep here. Will look into that. |
Co-authored-by: Francesco Pannarale <[email protected]>
|
Some notes for posterity. Francesco and I spent some time looking into why the ISSO interpolant is needed, since it adds quite a bit of complexity. We reproduced an earlier test from Andrew, which indeed indicates that generating the injections with the exact calculation takes ~10 min, versus just a few seconds using the interpolant. This, however, seems to contradict my own timing of the ISSO solver, which indicates that the exact calculation takes just about half a millisecond for a single point, and I struggle to believe the interpolant would be significantly faster than that. Bottom line: I propose we modify this PR to only include the exact ISSO calculation, and leave adding the interpolant as a smaller PR later, if the extra time is found to be really unacceptable. Meanwhile, I will do some profiling to understand where the time is coming from, and see if we can find a solution that does not involve carrying around a ~1 Mb precalculated file. |
|
I have been poking at the ISSO code a bit more, so here are a few more observations. First, I found that Next, I spent some time looking for a way to make the spline pickle file significantly smaller by making the interpolation grid coarser. In doing so, I realized that Anyway, after making that change, I found that I can reduce the spline pickle file to ~15 Kb only, and still obtain an interpolation better than 1e-5 almost everywhere (note that the spline proposed in the PR does not satisfy the condition that the error is below 1e-5 everywhere, contrary to what the docstring of Conclusion of this second note: I still propose we modify this PR to only include the exact ISSO calculation (I will look into this myself next). In one or two followup PRs, we will then address the high-spin problem in |
|
It seems I cannot open a PR against this PR, but the change I propose is on this branch: https://github.com/titodalcanton/pycbc/tree/embright_remove_isso_interp. |
Remove ISSO interpolator for now
Co-authored-by: Tito Dal Canton <[email protected]>
…eutron_stars package (gwastro#3937) * Remnant mass function moved from em_progenitors to conversions. Relevant NS functions moved to neutron_stars. Calls to these functions fixed to reflect new locations * no change, pushing to resolve issue with checkout * importing pycbc.tmpltbank rather than pycbc in attempt to resolve import issues (currently unresolved) * moved neutron stars code and eos data to new package and created __init__ file * Renamed em_progenitors as ns_functions and moved to neutron_stars package. Edited how said functions are imported elsewhere where necessary. Added neutron_stars package path to setup file * First suite of changes suggested in PR. Removes some cases of double imports and old code comments from during development * Removed the logging and sys calls. Changed the exception to a ValueError with clear error messages. Co-authored-by: Andrew R. Williamson <[email protected]> * retrigger checks * EM-bright work * Constraint string substitutions for static args * Speed up remnant mass calculation (1st attempt) * Adding derivatives in ns_functions for root solver * Add to comment * Missing bracket * Fix bugs * Enable spherical or cartesian spins for m_rem calc * Reorganising neutron_stars module * Some fixes to isso solver * Adding 2D interpolation for ISSO radii * Refined bivariate interpolator for ISSO calc * Change in setup.py, needed for tox * Fix docstrings * Fixing doc strings * Remove old pycbc_dark_vs_bright_injections code * Added newline at end of file to conform to codeclimate check * removed trailing whitespace * removed trailing whitespaces * Removed imports outside of toplevel * removed trailing whitespaces * Empty-Commit, retrigger checks * correctly importing pickle and os.path at top of pg_isso_solver.py * Creating 3 variables in which previously repeated lines of calculation are performed (in need of new names, potenatially better solution available) * fixed issue with unnecessary indents * changed variables to functions after negecting to realise they relied on called variables. Still in need of more informative names, potentially room for more elegant solution * removed trailing whitespaces * Fixing error in trig functions (missing return) * removed functions aimed at avoiding repeated code. Deemed unnecessary * rectified mislabeled parameter chi_lims to bounds as required by function * introducing latest version of remnant mass function in coord_utils get_randm_mass function. Minor rewrite to earlier portion sof function to acommodate this change * added short description of concat_grid function * Moved ns_sequence_file initialisation above it's first use to avoid error * fixed issue with methods used to call variables in remnant mass function * introduced eos as a variable in the get_random_mass function in order to work with updated remnant_mass funtion * adding new lines between function definitions * re-introduced eta_nsbh in get_random_mass as it is required later in the function * moved module level imports to top of init file * moved module level imports back to bottom of init file as moving to top seemed to break code * removed trailing whitespace * Initialised input_is_array variable and added functionality to check if input variable is an array. If it is, input_is_array is set to True * resolved issue with undefined variable erroneously added in code which checks if an input is an array * addrd r prefix to doctring in an attempt to resolve issues related to use of backslashes * Replace code which checks if input is an array with more reliable method Co-authored-by: Tito Dal Canton <[email protected]> * Updated docstrings to more accurately represent the forms of inputs and outputs in a number of functions * Fixed typo in docstring Co-authored-by: Tito Dal Canton <[email protected]> * Reformatted code to agree with codeclimate Co-authored-by: Tito Dal Canton <[email protected]> * Removed hardcoded output path for generate_isso_bivariate_interp() * Fix wrong import name * Updated comments in constraint function Co-authored-by: Francesco Pannarale <[email protected]> * Remove ISSO interpolant, will bring back a better version later * Forgot a few things * Update pycbc/tmpltbank/coord_utils.py Co-authored-by: Tito Dal Canton <[email protected]> Co-authored-by: Samuel Higginbotham <[email protected]> Co-authored-by: Andrew R. Williamson <[email protected]> Co-authored-by: Andrew Williamson <[email protected]> Co-authored-by: Tito Dal Canton <[email protected]> Co-authored-by: Francesco Pannarale <[email protected]> Co-authored-by: Tito Dal Canton <[email protected]>
…eutron_stars package (gwastro#3937) * Remnant mass function moved from em_progenitors to conversions. Relevant NS functions moved to neutron_stars. Calls to these functions fixed to reflect new locations * no change, pushing to resolve issue with checkout * importing pycbc.tmpltbank rather than pycbc in attempt to resolve import issues (currently unresolved) * moved neutron stars code and eos data to new package and created __init__ file * Renamed em_progenitors as ns_functions and moved to neutron_stars package. Edited how said functions are imported elsewhere where necessary. Added neutron_stars package path to setup file * First suite of changes suggested in PR. Removes some cases of double imports and old code comments from during development * Removed the logging and sys calls. Changed the exception to a ValueError with clear error messages. Co-authored-by: Andrew R. Williamson <[email protected]> * retrigger checks * EM-bright work * Constraint string substitutions for static args * Speed up remnant mass calculation (1st attempt) * Adding derivatives in ns_functions for root solver * Add to comment * Missing bracket * Fix bugs * Enable spherical or cartesian spins for m_rem calc * Reorganising neutron_stars module * Some fixes to isso solver * Adding 2D interpolation for ISSO radii * Refined bivariate interpolator for ISSO calc * Change in setup.py, needed for tox * Fix docstrings * Fixing doc strings * Remove old pycbc_dark_vs_bright_injections code * Added newline at end of file to conform to codeclimate check * removed trailing whitespace * removed trailing whitespaces * Removed imports outside of toplevel * removed trailing whitespaces * Empty-Commit, retrigger checks * correctly importing pickle and os.path at top of pg_isso_solver.py * Creating 3 variables in which previously repeated lines of calculation are performed (in need of new names, potenatially better solution available) * fixed issue with unnecessary indents * changed variables to functions after negecting to realise they relied on called variables. Still in need of more informative names, potentially room for more elegant solution * removed trailing whitespaces * Fixing error in trig functions (missing return) * removed functions aimed at avoiding repeated code. Deemed unnecessary * rectified mislabeled parameter chi_lims to bounds as required by function * introducing latest version of remnant mass function in coord_utils get_randm_mass function. Minor rewrite to earlier portion sof function to acommodate this change * added short description of concat_grid function * Moved ns_sequence_file initialisation above it's first use to avoid error * fixed issue with methods used to call variables in remnant mass function * introduced eos as a variable in the get_random_mass function in order to work with updated remnant_mass funtion * adding new lines between function definitions * re-introduced eta_nsbh in get_random_mass as it is required later in the function * moved module level imports to top of init file * moved module level imports back to bottom of init file as moving to top seemed to break code * removed trailing whitespace * Initialised input_is_array variable and added functionality to check if input variable is an array. If it is, input_is_array is set to True * resolved issue with undefined variable erroneously added in code which checks if an input is an array * addrd r prefix to doctring in an attempt to resolve issues related to use of backslashes * Replace code which checks if input is an array with more reliable method Co-authored-by: Tito Dal Canton <[email protected]> * Updated docstrings to more accurately represent the forms of inputs and outputs in a number of functions * Fixed typo in docstring Co-authored-by: Tito Dal Canton <[email protected]> * Reformatted code to agree with codeclimate Co-authored-by: Tito Dal Canton <[email protected]> * Removed hardcoded output path for generate_isso_bivariate_interp() * Fix wrong import name * Updated comments in constraint function Co-authored-by: Francesco Pannarale <[email protected]> * Remove ISSO interpolant, will bring back a better version later * Forgot a few things * Update pycbc/tmpltbank/coord_utils.py Co-authored-by: Tito Dal Canton <[email protected]> Co-authored-by: Samuel Higginbotham <[email protected]> Co-authored-by: Andrew R. Williamson <[email protected]> Co-authored-by: Andrew Williamson <[email protected]> Co-authored-by: Tito Dal Canton <[email protected]> Co-authored-by: Francesco Pannarale <[email protected]> Co-authored-by: Tito Dal Canton <[email protected]>
The functions within tmpltbank/em_progenitors.py serve purposes beyond template banks, for instance, they are used quite integrally in pygrb where the remnant mass function is used to separate potentially EM-bright injections from EM-dim injections. Here, I have moved the remnant_mass function to conversions.py so it may be used more broadly, and moved the rest of the em_progenitors.py functions to the new ns_functions.py (name subject to change) within a new package under the name neutron_stars. This package also now houses the ns_sequence data once stored in tmpltbank since it is more relevant to the functions stored here. The aim of this change is to allow for these functions to be used more broadly under a more appropriately named package.