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

Further LOFAR development #656

Open
wants to merge 188 commits into
base: develop
Choose a base branch
from
Open

Further LOFAR development #656

wants to merge 188 commits into from

Conversation

MijnheerD
Copy link
Collaborator

This is a draft PR to follow up on the newest additions from the LOFAR side. These should mainly concern LOFAR specific modules, except for some framework changes that might happen do accommodate LOFAR specific requirements.

karenterveer and others added 30 commits July 13, 2023 15:01
Included various sky models, moved sky noise generation out of run and into begin procedure
Including LBA inner and removed reference channels
So far, only LBA_inner and LBA_outer were implemented as antenna sets. This can be extended further in the future.
@anelles
Copy link
Collaborator

anelles commented Dec 19, 2024

It looks like this has gotten out of sync with the general branch. I suggest to really not keep PRs open for so long -- the further it gets the more annoying it is to merge later.
@MijnheerD @karenterveer @PhilippLaub can you resolve the merge conflicts and merge and then continue with smaller changes?

PhilippLaub and others added 2 commits December 19, 2024 13:26
add function to calculate average plane wave direction for an event
# Conflicts:
#	NuRadioReco/utilities/trace_utilities.py
@MijnheerD MijnheerD marked this pull request as ready for review December 19, 2024 15:14
@MijnheerD
Copy link
Collaborator Author

It looks like this has gotten out of sync with the general branch. I suggest to really not keep PRs open for so long -- the further it gets the more annoying it is to merge later. @MijnheerD @karenterveer @PhilippLaub can you resolve the merge conflicts and merge and then continue with smaller changes?

All of the conflicts should be artificial, in the sense that it probably results from this branch behind on develop. Github has some issues with tracking updates in PRs when both the origin and target branches change, so this might not show up correctly. Any changes that come from this branch should only touch LOFAR specific files.

The idea was to have this PR be a draft until we had a pipeline. Seeing we recently got to a state where we can run our pipeline, I guess we can convert it to a real PR and merge :)

@MijnheerD
Copy link
Collaborator Author

MijnheerD commented Dec 19, 2024

@cg-laser @anelles @sjoerd-bouma To help you with the review process, the only non-LOFAR files that were touched are:

  • detector_base.py : added the get_antenna_mode() function which retrieves the "antenna_mode" keyword from the detector description. In the context of LOFAR we use this keyword to distinguish the channels connected to different antenna sets.
  • antennapattern.py: the preprocess_LOFAR_txt() function has been changed to allow to specify X/Y dipole when generating the antenna models (this is necessary because somehow the old models were different for the two orientations)
  • channelGalacticNoiseAdder.py: these changes are from the feature/galacticNoiseAdderUpgrade branch, which I think are being included in galactic noise adder upgrades #740 (?)

@MijnheerD
Copy link
Collaborator Author

I decided to make a new section called version 3.1.0-dev in the changelog.txt 😇 If you agree with this naming scheme, then everything should be ready for review now!

Copy link
Collaborator

@anelles anelles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not super trivial to review such a huge PR. A couple of (I hope) smaller points to be fixed.

NuRadioReco/detector/LOFAR/signalchain/RCU_gain_new_5.txt Outdated Show resolved Hide resolved
NuRadioReco/detector/detector_base.py Outdated Show resolved Hide resolved

out = beamformer(fft_traces, freq, delays)
timeseries = fft.freq2time(out, 200 * units.MHz) # TODO: is this really necessary?
my_direction_cartesian = hp.spherical_to_cartesian(theta, phi)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, I like variables with my_ ... this sounds like a hack (without having tested this).

Copy link
Collaborator Author

@MijnheerD MijnheerD Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean? This variable should be local to the negative_beamed_signal() function, and my standard way of not interfering with variables from the outer scope is to append "my_" in front.

@MijnheerD
Copy link
Collaborator Author

Thanks for all the comments Anna! The intent was indeed not for you to go over all the individual LOFAR modules, as these should have been reviewed internally already. The most important thing is that we don't break existing NRR functionality. But I very much appreciate the comments you left :)

@MijnheerD
Copy link
Collaborator Author

As an aside, my goal was to also document the LOFAR modules and files more clearly in the LOFAR documentation, that @sjoerd-bouma started in #767

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.

6 participants