Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable modifications of the RNO-G detector description #686

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fschlueter
Copy link
Collaborator

I all,

with this PR I want to start a discussion on how we will support the feature to modify the RNO-G detector description "on-the-fly". The feature is useful for systematic studies and in our efforts to improve the detector description. On example: We want to rotate the antenna orientations to see the effect of this rotation to the simulation of the galactic emission. In the future it will come in handy to allow to replace the response function of a channel with something different.

This PR adds a very simple function to the detector class which should (untested!) allow you to replace a certain value in the detector description. It is not very user friendly in the sense that you really need to know what you have to do. We could think of more specific user friendly interfaces. It could be a good possibility to create an inherent class which hosts all these modification interfaces we envision.

Any suggestions / comments / opinions?

@philippwindischhofer
Copy link

Hi @fschlueter thanks for starting the discussion on this topic! Below are a few quick comments and questions from my side. If I can think of anything else, I will post another comment.

Station geometry storage and bookkeeping

  1. Is the long-term plan to store the station / channel geometry information also in the hardware DB? (I can already see some geometry-related collections filled, so I guess the answer is "yes"?) If so, I would advocate for removing the current json files as soon as the DB-based solution is fully available.
  2. The DB has the nice "interval of validity" (IOV) mechanism to allow multiple geometries to be available for non-overlapping times. When we iterate on the geometry calibration later this year (and in the longer-term future), we might have to update the geometry as new calibration data becomes available. If our best estimate for the station geometry changes over time, I wonder if the IOV is the best mechanism to handle that? E.g. we might want to keep multiple past calibration results around to easily check how reconstruction / simulation results change. One way to implement this would be to build a poor-man's commit-tree by storing for each station geometry also a "parent" that points to the geometry that it corrects / updates. (I don't know if having multiple overlapping IOVs would break things elsewhere.)

On-the-fly updates of station geometry

  1. I guess we might want to support two separate interfaces: an "expert" one that allows the user to modify all degrees of freedom in an unconstrained manner as they please; and a "user" one that provides a more high-level interface. In the latter case, only a few "knobs" would be exposed that internally map onto antenna positions. For example, if we decide to go for the eigenvector parametrization of geometry calibration results that I presented at the virtual collaboration meeting, the exposed DOFs would be the nuisance parameters corresponding to the eigenmodes.

Visualization

  1. Do we already have some centralized code to visualize a given station geometry? I believe this could be worthwhile to build intuition for how a particular update to the geometry actually changes the situation in the ice. Right now, I think people write their own scripts (I have one, the calibration code has one, and I think @MatthewMarsee also has one), and it might be worth harmonizing these.

@fschlueter
Copy link
Collaborator Author

Hey @philippwindischhofer some quick answers:
To 1): Yes and Yes :)
To 2): We have anticipated for what you describe and the DB / NuRadio class already accommodates for this. A longer explanation is here but the short version is: With the database_time you can select a measurement of e.g. the channel position which was primary at that specific time while in the mean time with have specified a new, supposedly better measurement, as the primary one.
To 3): Sounds reasonable and I guess we can add the user-friendly interface along the road as we need it. Should be but that in a dedicated class though?
To 4): I am not terrible aware of all the different visualisation codes neither (maybe @sjoerd-bouma can help us out here). But I remember that we have a detector browser viewer?

@philippwindischhofer
Copy link

Hi @fschlueter, thanks for your rapid answer!

  1. Brilliant!! I wasn't aware of this.
  2. Yes, I would put all that into dedicated classes to keep it nice and separate. A future GeometryCalibration class would then take as inputs the database_time and values for the nuisance parameters, and return the properly calibrated detector geometry.

@fschlueter
Copy link
Collaborator Author

Yes, I would put all that into dedicated classes to keep it nice and separate. A future GeometryCalibration class would then take as inputs the database_time and values for the nuisance parameters, and return the properly calibrated detector geometry.

Do you plan to perform calibrations "on-the-fly". I thought the idea is to run a calibration analysis and put the results of this in the DB

@philippwindischhofer
Copy link

Yes, the calibration would be run once, recorded in the DB, and a particular calibration retrieved via the database_time mechanism.

As part of the calibration we will get uncertainties on the station geometry. If we want to propagate the geometry uncertainties into a simulation / reconstruction (especially at the beginning, where the uncertainties will be large), we need a mechanism to sample the space of allowed station geometries. That part, I think, would profit from being done "on the fly" (since it is fast), instead of storing many explicit variations of the geometry in the DB.

@sjoerd-bouma
Copy link
Collaborator

@philippwindischhofer Re 4) - there's a nice interactive 3D visualization of NuRadio detectors (see the documentation for some more information on how to use it). It probably needs an update in order to also support the RNO-G detector, though; right now it only works with jsons, and the interface is slightly outdated.

@fschlueter
Copy link
Collaborator Author

Yes, the calibration would be run once, recorded in the DB, and a particular calibration retrieved via the database_time mechanism.

As part of the calibration we will get uncertainties on the station geometry. If we want to propagate the geometry uncertainties into a simulation / reconstruction (especially at the beginning, where the uncertainties will be large), we need a mechanism to sample the space of allowed station geometries. That part, I think, would profit from being done "on the fly" (since it is fast), instead of storing many explicit variations of the geometry in the DB.

I see. Probably we should think of a smart way to allow shifting of positions and orientations.

@shallmann
Copy link
Collaborator

Thanks for starting this, Felix!

  • The detector validity is not affected by this, i.e. running a detector.update() would not re-set the values to the original. Maybe we need to keep this in mind (and might want to introduce a force_update or a is_modified somewhere to check if the detector is modified or not and be able to get back to the original if need be.

  • Having the possibility to modify detector descriptions will be very useful for sure. In addition to the general modify_channel_description function we can add more specialised functions to shift/rotate individual channels as we need them.

  • I like the suggestion @philippwindischhofer to be able to automatically randomly shift detector components by their allowed uncertainties. I guess not but: Are there already fields for uncertainty in the database?

  • If we have no other volunteers, I can look into updating the detector visualisation next week.

@sjoerd-bouma
Copy link
Collaborator

Tangentially to Steffen's comment and as was suggested by Christian in #616, I think it's important to be able to distinguish a modified detector from an unmodified detector. If we don't introduce a separate 'ModifiedDetector' class I think it would be good to at least have a (one-time) warning issued when a user first modifies the detector, and maybe an is_modified property as Steffen suggests to keep track of this. Then in terms of being able to store/keep track of modified values I guess exporting to json still works?

@philippwindischhofer
Copy link

Hey @fschlueter and @shallmann,

I see. Probably we should think of a smart way to allow shifting of positions and orientations.

I like the suggestion @philippwindischhofer to be able to automatically randomly shift detector components by their allowed uncertainties.

For at least some cases (calibration being one of them) a proper treatment of the uncertainties will be a bit more complicated than simply "randomly wiggle antennas around independently". Rather, each uncertainty component will move around a set of antennas in a particular way. This is what I called "eigenmodes" earlier.

I guess not but: Are there already fields for uncertainty in the database?

I don't think there currently are---but @fschlueter or @jakobhenrichs would know for sure. Given the above, I think an important aspect will be to decide on the format we choose to store the "modifications" in. If we want this to go into the DB, it should be sufficiently general to cover all our future use cases.

Hey @sjoerd-bouma,

Tangentially to Steffen's comment and as was suggested by Christian in #616, I think it's important to be able to distinguish a modified detector from an unmodified detector.

I would even go one step further and, instead of just tagging a detector geometry as "unmodified" and "modified", give them more informative "tags". There would be a single "nominal" detector geometry that represents the best-estimate of the actual as-deployed instrument, and then there would be several named variations around that nominal case.

If we don't introduce a separate 'ModifiedDetector' class I think it would be good to at least have a (one-time) warning issued when a user first modifies the detector, and maybe an is_modified property as Steffen suggests to keep track of this. Then in terms of being able to store/keep track of modified values I guess exporting to json still works?

I don't have any opinion on whether a ModifiedDetector class is the way to go, or whether the current Detector class should hold this functionality. I would just advocate to make sure that the interface is the same for both, e.g. querying channel positions etc. should work in exactly the same way for both, so that we can use a modified detector in all locations where we can use an unmodified one. That's the essential feature that will make rigorous propagation of geometry uncertainties possible.

@shallmann
Copy link
Collaborator

For at least some cases (calibration being one of them) a proper treatment of the uncertainties will be a bit more complicated than simply "randomly wiggle antennas around independently". Rather, each uncertainty component will move around a set of antennas in a particular way. This is what I called "eigenmodes" earlier.

I didn't fully appreciate the meaning of 'eigenmodes' earlier, yes you are right. We need to agree on which uncertainties and correlations / eigenmodes we need in the database. Independent per-channel uncertainties would have been a no-brainer to add to the database, eigenmodes may need a bit more thought.

Given the above, I think an important aspect will be to decide on the format we choose to store the "modifications" in. If we want this to go into the DB, it should be sufficiently general to cover all our future use cases.

I would even go one step further and, instead of just tagging a detector geometry as "unmodified" and "modified", give them more informative "tags". There would be a single "nominal" detector geometry that represents the best-estimate of the actual as-deployed instrument, and then there would be several named variations around that nominal case.

  1. We intended the database to hold new "best fit" positions, and (eventually) uncertainties/eigenmodes after each iteration of calibrating the detector (each iteration retrievable by specfying a database_time when initialising the detector object). I think this is what we definitely want in the official database.
  2. Another use case is to deliberately shift/delay/rotate a set of channels for individual studies. We should have a functionality to allow for that by passing a modification dict or applying some modification functions. I think this is what Felix already had in mind when starting this thread.
  3. We can discuss on wether (and how) we need official fixed named modification sets stored centrally (which cannot be regenerated by a function using the official best fit database values and uncertainties).
  4. A simple variable to tell if a detector was manipulated will be useful irrespective of that.

I don't have any opinion on whether a ModifiedDetector class is the way to go, or whether the current Detector class should hold this functionality. I would just advocate to make sure that the interface is the same for both, e.g. querying channel positions etc. should work in exactly the same way for both, so that we can use a modified detector in all locations where we can use an unmodified one. That's the essential feature that will make rigorous propagation of geometry uncertainties possible.

No strong opinion either, but if we opt for a separate ModifiedDetector class this should derive from the unmodified detector of course to ensure all queries still work the same. Keeping both a modified and an unmodified detector alive (which afaik may be not possible with two normal detectors since they are a singleton?!?) would allow for direct comparisons between the two, if there is a use case

@philippwindischhofer
Copy link

Independent per-channel uncertainties would have been a no-brainer to add to the database, eigenmodes may need a bit more thought.

Yep! Although, if we find a good way of keeping the eigenmodes in the DB, then independent per-antenna uncertainties are just a special case :-)

I fully agree with your points 1-4!

Keeping both a modified and an unmodified detector alive (which afaik may be not possible with two normal detectors since they are a singleton?!?) would allow for direct comparisons between the two, if there is a use case

Indeed! I think this is a good argument in favor of a ModifiedDetector. And if we generate the ModifiedDetector through a call to the original Detector like Detector.modify(mod), then we can also easily loop over all variations (including the nominal case).

@shallmann
Copy link
Collaborator

shallmann commented Jun 12, 2024

  • If we have no other volunteers, I can look into updating the detector visualisation next week.

Added as much as possible given the current state of the RNO-G detector in this PR: #690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants