Skip to content

Draft: Proposal : Simplified API

Thomas Sainrat requested to merge thomas.sainrat/ligo.skymap:new-api into main

Disclaimer

First and foremost, this MR is meant as a proof of concept ; it is incomplete and SHOULD NOT be merged in the current state. It aims to propose a simplified API for the package, and currently adds a file (partially) implementing this API ; it does not induce any breaking change this way, but the ultimate goal would be to actually modify the rest of the package on the long-term. This would likely result breaking changes (although there may be ways to keep the current behavior working), meaning this would likely lead to a version 2.0 of the package, and require strategies to allow users to adapt their code to the new version.

Rationale

In its current state, the user of the ligo.skymap package is currently confronted to several different types of skymap representations:

  • the regular HealPIX ("fixed-ordering") format, typically in the form of a numpy array (but can also be an astropy Table) ; it can have two different orderings, NESTED and RING.
  • the newer MOC format, typically as an astropy Table or a numpy structured array. As a user of the package who have been working with the different functions (plotting, reading and writing, postprocess), I have been confused multiple times on which representation should be used ; I have several times been mistaken between the NESTED and RING types (which are only distinguished by metadata), and have spent some time finding how to convert between MOC and fixed-ordering formats, which is not well documented currently. This is not aided by the fact that most functions will only accept one of the formats, which is not consistent across all functions (for instance, a non-trivial operation is to generate a skymap with bayestar.localize (returns MOC format) and apply find_greedy_credible_levels (takes in the fixed-ordering format)).

Proposal

This PR introduces a new Skymap class, which can be instantiated from any of the aforementioned skymap representations, and provide convenience utilities to convert between the different formats. The changes to the API would require that any function currently receiving a skymap (in either format) should now take in the new Skymap object. This is all implemented in the ligo.skymap.skymap module (for a lack of a better name) ; it contains the Skymap class, some of the functions from other modules wrapped (almost all of postprocess, bayestar.localize, the read and write function from moc), as well as a wrapper around ligo.skymap.plot providing similar functionality (adding the relevant matplotlib projections). Examples of usage can be found in the main module tests folder.

Possible path forward

This MR is mostly meant as a proposal ; it does not require an immediate acceptation, and I am fully opened to other alternatives. It has not been extensively tested aside from the included pytest files. In the case where we want to move forward with this, there are several possible options :

  • keep the same format as the MR, with a separate file containing the simplified API, and advertising it to users ; this requires wrapping the currently unwrapped functions, which is a small amount of work, as well as documenting it.
  • proceed with modifying the current API in a non-backward compatible way. This should trigger a 2.0 version of the package ; existing users would need to restrict themselves to the 1.x versions to keep using the old API. A period where both the 2.x and 1.x versions would be updated might be necessary.
  • find a way to keep backward compatibility, by keeping the current API intact while also allowing for the new Skymap object to be used. This requires more work than the other options, and would introduce more code into the package, but would keep existing codes functioning ; it may also be a transition period with the previous option, raising warnings for users to use the new API. EDIT : I may have some ideas on how this could be done with a limited amount of code, so this may be easier than I thought.
Edited by Thomas Sainrat

Merge request reports

Loading