ML4GW / aframe

Detecting binary black hole mergers in LIGO with neural networks
MIT License
6 stars 16 forks source link

Error handling when using 'H1', 'V1' as ifo's #174

Closed sjhend03 closed 4 months ago

sjhend03 commented 5 months ago

Changed L1 to V1' insandbox.cfgand addedv1toLigoResponseSetandLigoWaveformSetand commented outl1, ran sandbox pipeline and returned, Traceback (most recent call last): File "/law_forward/py/luigi/worker.py", line 210, in run new_deps = self._run_get_new_deps() File "/law_forward/py/luigi/worker.py", line 138, in _run_get_new_deps task_gen = self.task.run() File "/opt/aframe/aframe/tasks/data/waveforms.py", line 199, in run waveform_set = LVKWaveformSet(**parameters) File "", line 24, in init File "/opt/aframe/libs/ledger/ledger/injections.py", line 225, in post_init InjectionMetadata.post_init(self) File "/opt/aframe/libs/ledger/ledger/injections.py", line 41, in post_init super().post_init() File "/opt/aframe/libs/ledger/ledger/ledger.py", line 52, in __post_init__ raise ValueError( ValueError: Field l1 has 0 entries, expected 50`

wbenoit26 commented 5 months ago

Right, looks like we don't allow fields to have different lengths. I think in general that's good behavior, but maybe the cleanest thing to do will be to allow fields of length 0, so just changing the linked line to elif len(value) != _length and len(value) != 0:. With that, we can just put whichever ifos we want in the LVKResponseSet class. Though, there may be other parts of the Ledger class and subclasses that expect equal-length arrays, so we should keep an eye out for other errors.

The alternative would be to have a function that creates a class based on a list of detectors. That would adhere more closely to the original idea of the object, but I think it would make them so cumbersome to use that it wouldn't be worth it.

sjhend03 commented 5 months ago

I like the first idea, I'll run through the code with that method

wbenoit26 commented 5 months ago

It looks like @EthanMarx has put together a PR (#175) that will handle this ledger generalization pretty nicely. I think keep doing what you're doing, but once that gets merged in you can switch over.

sjhend03 commented 5 months ago

Perfect, sounds good.

EthanMarx commented 4 months ago

@sjhend03 If you pull the latest changes from main, things should work out of the box by just setting ifos = ["H1", "L1", "V1"]

EthanMarx commented 4 months ago

You will have to set the AFRAME_REPO environment variable to point to the location where you cloned aframev2

sjhend03 commented 4 months ago

Sounds good thanks!

EthanMarx commented 4 months ago

@sjhend03 going to close this, but feel free to re-open other issues as they arise.