kazewong / jim

Gravitational-wave data analysis tools in Jax
MIT License
59 stars 19 forks source link

Added code to reverse transforms for more flexibility #132

Closed ThibeauWouters closed 2 months ago

ThibeauWouters commented 3 months ago

New attempt at improving the flexibility and use of "pre-made" transforms such as masses transforms etc.

Source code changes

I have added two utility functions:

These functions are then used to simplify the "pre-made" transforms in single_event/transforms.py to be shorter and clearer.

Test changes

thomasckng commented 3 months ago

@ThibeauWouters Thanks for the PR! The changes look good!

For the tests, I think the test scripts in test/ are meant to be light-weighted, as they will run whenever there is a new commit. They meant to check if everything is working instead of generating meaningful result. A example with optimized parameters are meant to be put in example/. I am canceling the test runs now.

ThibeauWouters commented 3 months ago

@thomasckng Thanks! I had local copies of the test file to try more agressive hyperparameters but indeed I seemed to have messed them up with the ones in the test directory. Thanks for canceling the runs.

ThibeauWouters commented 3 months ago

@kazewong I adapted the code to adapt to your previous comment. But now it feels a bit off in the sense that the "predefined" transforms are classes, that have to be instantiated in order to get the actual transform to be used in PE, and the reverse transforms that are created from the reverse function are objects/instances already. The former require users to give name_mapping, which as I mentioned earlier on is a bit redundant since we would like the name mapping to be fixed for these classes, whereas the former doesn't. So the predefined transform and its reverse are not treated equally in the current implementation.

kazewong commented 3 months ago

@kazewong I adapted the code to adapt to your previous comment. But now it feels a bit off in the sense that the "predefined" transforms are classes, that have to be instantiated in order to get the actual transform to be used in PE, and the reverse transforms that are created from the reverse function are objects/instances already. The former require users to give name_mapping, which as I mentioned earlier on is a bit redundant since we would like the name mapping to be fixed for these classes, whereas the former doesn't. So the predefined transform and its reverse are not treated equally in the current implementation.

There are a couple of things you mentioned here:

  1. Re: predefined transform and its reverse are not treated equally: I don't think they should. Inverting a transform target a bijective transform, and predefined transforms are a subset of bijective transforms. For example, reverse_bijective_transform(ScaleTransform((['x'], ['y']), 1.0)) should return a valid transform. As long as this is the interface the user will use, I think this is legit.
  2. If we later establish predefined transform can be called with PredefinedTransform(), the interface makes sense to me still, which will be replacing PredefinedTransform() with reverse_bijective_transform(PredefinedTransform()) at the same spot.

I think this is more ergonomic for the user, instead of needing to use a function to call the predefined transform.

Going forward, I do think predefined transforms that enforce the naming strictly should mostly be constructed using the interface PredefinedTransform(). Only transform that does not enforce naming such as ScaleTransform should allow inputs other than the transform parameters.

ThibeauWouters commented 2 months ago

@thomasckng @kazewong I have updated the PR a bit after the discussion yesterday. Let me know if this looks OK to you or if further changes are required.

ThibeauWouters commented 2 months ago

@thomasckng The files have been deleted

ThibeauWouters commented 2 months ago

@kazewong I have updated the source code as requested -- is this what you were referring to?