Generalise table preprocessing#2791
Conversation
|
Overall looks good, however I'm wondering if this preprocessing module should go under |
This comment has been minimized.
This comment has been minimized.
LukasBeiske
left a comment
LukasBeiske
left a comment
There was a problem hiding this comment.
Besides that, looks good to me!
Kind of related #2789
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
It's ok for me. |
| calculate_event_weights, | ||
| ) | ||
| from pyirf.utils import calculate_source_fov_offset, calculate_theta | ||
| except ModuleNotFoundError: |
There was a problem hiding this comment.
I think with this many imports, it's simpler to do something like this:
try:
from ...
has_pyirf = True
except ModuleNotFounderror:
has_pyirf = False
and then raise on has_pyirf is False.
Hi @maxnoe , @kosack , I had commented above, after Karl's comments, that some of these comments are out of the scope of what this PR aims. I didn't get a reply, if you agree or not. So what do you think? |
|
At least the request for issuing a warning and for proper variable names should be addressed. |
mexanick
left a comment
mexanick
left a comment
There was a problem hiding this comment.
It looks to me the urgent requests has been addressed. I'd prefer to implement the rest in the separate PRs to have them easier to digest and review.
|
|
||
| if SimulatedEventsInfo is None: | ||
| raise OptionalDependencyMissing("pyirf") | ||
| raise ImportError("pyirf is required for this functionality") |
There was a problem hiding this comment.
Why did you replace the correct usage of the OptionalDependencyMissing exception here?
|
@kosack please have another look here |
also, shouldn't it be bumped in priority in DataPipe Followup to be merged before DL3 production? |
|
Looks like sonar couldn't find the coverage report:
|
|
I restarted the sonar job, the artifact exists |
|
The error is that a merge happened between the CI run here creating the coverage report and the running of the sonar job, resulting in a mismatch of line numbers. I merged main here again, that should fix it. |
|
kosack
left a comment
kosack
left a comment
There was a problem hiding this comment.
Looks good now, thanks for addressing my comments.
6 participants
Closes #2790
Refactoring the DL2 processing for producing IRFs. The motivation is to use the same code for more usecases that process DL2 data, e.g. the telescope cross calibration.
The refactored module is the irf/preprocessing.py
The default behaviour of the preprocessing remains the same as before.