Beersheba crashes under certain circumstances when detector_db isn't given. about ic HOT 12 CLOSED

Does doing this invalidate the test? As far as I can tell it doesn't,

I didn't see this before my previous message. No, it doesn't!

from ic.

Does IC have a universal 'cities' catch-section that checks for required arguments

Yes, and it's already implemented. I would have to check every city, but it seems that Beersheba has already detector_db as a required argument. The problem seems to be that we have a sneaky bit of code

Should I only do this for Beersheba & key cities as it may mess with others?

If I'm right about the above, this will have an implication in every city.

from ic.

Removing those three lines and trying to run Beersheba without a detector_db variable assigned in the config returns the error:
ValueError: The function 'beersheba' is missing an argument 'detector_db' of type 'str'.

This is the expected behaviour, and would avoid the issue mentioned in my first comment. Should I check every city to see if this has any sort of adverse behaviour on them?

It doesn't however resolve the issue that if you somehow generate an event outwith your detector geometry, Beersheba will return a nonsense error. I'm not sure if this is a relevant issue, as it's a bit of an edge-case, and I'm assuming previous cities have checks for this sort of thing before Beersheba.

from ic.

Should I check every city to see if this has any sort of adverse behaviour on them?

Yes, please.

if you somehow generate an event outwith your detector geometry, Beersheba will return a nonsense error
I'm assuming previous cities have checks for this sort of thing before Beersheba.

I don't think we have this kind of checks as we have been always working with the same detector. This will require an ad-hoc solution for each city, I am afraid. It would be great if you can take care of that :)

from ic.

First thing I've noted is that doing this increases the errors from the tests from:
1 failed, 1105 passed, 17 skipped, 4 xfailed, 9 warnings in 125.94s (0:02:05)
To:
19 failed, 1087 passed, 17 skipped, 4 xfailed, 1 warning in 128.80s (0:02:08)

All the new errors are from esmeralda, beersheba, isaura and 'writer_test_city' not having the argument detector_db provided, so I can see where it's now required and resolve these explicit errors.

There may be more discrete errors, but I'm not sure how to gauge those.

from ic.

I can see where it's now required and resolve these explicit errors

excellent!

from ic.

Simply adding detector_db = 'new' to Esmerelda, Isaura, and Beersheba's config files resolves most of the errors. The trickier error appears to be with writer_test_city. The remaining errors occur due to the function compare_signature_to_values here, which makes sure that the signature of the city matches that which is passed through to it.

Because I've removed the lines from the city class that provided the default detector geometry shown here, this test imports the city structure from components.py, but cannot find a detector_db value to pass-through as it would have previously.

I'm currently trying to think of an elegant solution for this, but haven't come up with anything neat other than providing writer_test_city with a default detector_db value, which I feel may cause problems (whats the point of the test if you provide it with the value it needs to succeed outwith the input?)

from ic.

For example, if I alter this line to include detector_db = 'new' as an argument, rather than relying on it being passed through from components.py as was done previously, the errors disappear.

Does doing this invalidate the test? As far as I can tell it doesn't, as the purpose of the test is to see if the different writers open the file-type as expected (hits_writer contains the "RECO" group, kr the "DST" group), but I may be misinterpreting what this test does.

from ic.

The remaining errors occur due to the function compare_signature_to_values here,

It would be good if you could open the PR already so we could see your changes more directly and the tests failures directly in CI. (and continue the conversation over there).

but haven't come up with anything neat other than providing writer_test_city with a default detector_db value

which is perfectly fine. This test needs to provide a value with the new implementation. The only reason it was not doing so is because there was a default value (because NEW was the only detector until then). The fact that the test is failing is telling you that the call signature to a city has changed, because that's what you did.

whats the point of the test if you provide it with the value it needs to succeed outwith the input?

This is the equivalent of having a function f(x) with a test test_f() that calls f. Imagine that later you change f to be f(x, y). What's the point of changing test_f? Well, test_f needs to call f with the correct signature, so if you change the signature of f, the test must also be updated. You are not necessarily changing what you test, you are just updating the test to reflect the new signature of f.
In the specific case you are dealing with, writer_test_city was receiving detector_db = "new" from the city decorator. Now, this decorator no longer does that, and detector_db must be provided by the caller, so the test must provide the same value that was being provided before in order to maintain its logic.

from ic.

Sorry for the delay, I've made the pull request now.

from ic.

With detector_db now required as an argument here, this issue can be closed I think.

You may still have weird edge cases if events somehow make it to beersheba while being outwith the detector dimensions, but I don't think that should be possible within IC.

from ic.

This should have been closed automatically, but the syntax works only for the master branch.

from ic.