search

bilby-dev / bilby

A unified framework for stochastic sampling packages and gravitational-wave inference in Python. Note that we are currently transitioning from git.ligo.org/lscsoft/bilby, please bear with us!
https://bilby-dev.github.io/bilby/
MIT License
59 stars 66 forks source link

Why are there so many helper functions in waveform_generator_test.py? #566

Closed bilby-bot closed 3 weeks ago

bilby-bot commented 4 years ago

In GitLab by @git.ligo:moritz.huebner on Jul 17, 2020, 18:24

See e.g. https://git.ligo.org/lscsoft/bilby/-/blob/master/test/waveform_generator_test.py#L847 and below Some of these should just be moved into the utility module. Some others look like they are just reimplementations of existing code? @git.ligo:gregory.ashton

bilby-bot commented 4 years ago

In GitLab by @git.ligo:gregory.ashton on Jul 20, 2020, 07:39

@git.ligo:moritz.huebner I don't recall writing them (I'm 50/50 on if I did or someone else did). But, the re-implementations may be useful. It could be that they are a "de facto" way of integrating with LALSim, this way if someone changes the code in bilby itself, we can check if the changes still reproduce the "de facto" way of interacting. I'm not saying refactoring isn't worthwhile, but there could be a good reason for this.

bilby-bot commented 4 years ago

In GitLab by @git.ligo:moritz.huebner on Jul 20, 2020, 13:05

Git says you made this change @git.ligo:gregory.ashton Git says you made this change.

I can have a look through these functions and see what is going on exactly. Things like overlap calculation and strain normalization should definitely live in the utility module if they are required.

bilby-bot commented 4 years ago

In GitLab by @git.ligo:colm.talbot on Jul 20, 2020, 14:58

I'd be happy to move the overlap function, or a version of it, into the gwutils. I don't think the others are necessarily worth having, except for testing.

bilby-bot commented 4 years ago

In GitLab by @git.ligo:gregory.ashton on Jul 21, 2020, 12:12

Ah now I recall. The original work was added by @git.ligo:khun.phukon, I simply did the job of putting it into git: https://git.ligo.org/lscsoft/bilby/-/issues/400

bilby-bot commented 4 years ago

In GitLab by @git.ligo:moritz.huebner on Jul 22, 2020, 15:13

Fair enough, it may be worth putting these tests into a separate file so that the more atomic unit tests are separate.

bilby-bot commented 4 years ago

In GitLab by @git.ligo:moritz.huebner on Sep 8, 2020, 06:31

This has been resolved