Documentation / poetry improvements and fixes #773

sjoerd-bouma · 2024-11-29T14:44:54Z

I suppose technically this could have been two PRs but no functionality should be affected.

Optional dependencies weren't really implemented correctly in pyproject.toml (I accept full responsibility). This made very little difference, but the advantage is they can now be installed also using the conventional pip syntax pip install nuradiomc[documentation, proposal] to install the correct optional dependencies. Previously, optional dependencies were implemented as developer dependencies, and would never be installed during pip installation.
The rest is minor fixes to the documentation: some missing docstrings from the recent refactoring, and an attempt to somewhat tidy up the new 'overview of times' page, though I find the content somewhat unclear still. The table of contents on code documentation pages now also only shows top-level submodule and subpackage names instead of every method of every class inside them, which I think improves clarity,

…mprove-optional-deps

documentation/source/NuRadioReco/pages/times.rst

…mprove-optional-deps

sjoerd-bouma · 2025-01-13T12:18:21Z

ping @fschlueter - I hope you're happy with the changes. I think I have correctly described the way trigger times work as well in times.rst as well as the trigger docstrings. If/once we deprecate the triggerTimeAdjuster (#763) its section can be removed.

NuRadioReco/framework/trigger.py

fschlueter · 2025-01-13T12:43:51Z

A general comment/question. You are often always omitting the "." at the end of a sentence. I assume this is on purpose? Is there a PEP rule/suggestion on this? Similarly, you do not start sentences with a capitalised letter. Both of that I would do but I do not care much if we just try to be consistent with that in the future (or decide intentionally to be not consistent with that)?

documentation/source/NuRadioReco/pages/times.rst

fschlueter · 2025-01-13T12:51:44Z

documentation/source/NuRadioReco/pages/times.rst

+To avoid this behavior we often use the following procedure, through the `NuRadioReco.modules.channelStopFilter` module:

 We smoothly filter the first 5% and last 5% of the trace using a Tukey window function. This is a function that goes smoothly from 0 to 1.
-To avoid rollover of the pulse, we add 128ns of zeros to the beginning and end of the trace. Steps 2) and 3) are performed by the channelStopFilter module
-Both electric fields and channels have a trace_start_time variable. The get_times() function will automatically add the trace_start_time to the time array.
+To avoid rollover of the pulse, we add 128ns of zeros to the beginning and end of the trace.


Is that really true?

https://nu-radio.github.io/NuRadioMC/NuRadioReco/apidoc/NuRadioReco.modules.channelStopFilter.html#module-NuRadioReco.modules.channelStopFilter

It is what the channelstopfilter does. Whether it is actually used in practice or not is up to the user, I think : )

Yes, but how it is written within the docu suggests that it is the common/default behaviour. Maybe we can add this module to the list of modules which change the timing?

It is/was already there. As an aside, it doesn't do what it says here (the trace start time is not adjusted), but I will make a separate bugfix for that.

documentation/source/NuRadioReco/pages/times.rst

sjoerd-bouma · 2025-01-13T12:59:19Z

A general comment/question. You are often always omitting the "." at the end of a sentence. I assume this is on purpose? Is there a PEP rule/suggestion on this? Similarly, you do not start sentences with a capitalised letter. Both of that I would do but I do not care much if we just try to be consistent with that in the future (or decide intentionally to be not consistent with that)?

I agree. Though I would like to point out that times.rst was not written by me and I'm just trying to clean it up a bit. I also don't think it's worth getting too upset over typos, missing capitals or full stops in doc strings (and I don't think it's necessary to enforce some kind of standard), though obviously correctly written and formatted text is nicer to read.

fschlueter · 2025-01-13T13:00:59Z

A general comment/question. You are often always omitting the "." at the end of a sentence. I assume this is on purpose? Is there a PEP rule/suggestion on this? Similarly, you do not start sentences with a capitalised letter. Both of that I would do but I do not care much if we just try to be consistent with that in the future (or decide intentionally to be not consistent with that)?

I agree. Though I would like to point out that times.rst was not written by me and I'm just trying to clean it up a bit. I also don't think it's worth getting too upset over typos, missing capitals or full stops in doc strings (and I don't think it's necessary to enforce some kind of standard), though obviously correctly written and formatted text is nicer to read.

I was not referring to `times.rst' but the doc-strings in function definitions.

fschlueter · 2025-01-14T14:25:54Z

pyproject.toml


 [tool.poetry.extras]
 documentation = ["Sphinx", "sphinx-rtd-theme", "numpydoc"]
 proposal = ["proposal"]
 galacticnoise = ['pygdsm']
 ift_reco = ['nifty5', 'pypocketfft']
-muon_flux_calc = ['MCEq', 'crflux']
-RNO_G_DATA = ["mattak", "runtable", "pandas"]


Why removing runtable and pandas from the RNO_G_DATA dependencies?

I mean, frankly, I don't remember. But runtable is not a public dependency so will fail (and the installation will error out) for some users (probably mainly RNO-G users that don't have their git profile set up via ssh), whereas reading RNO-G data is in theory possible for everyone with mattak only.

I've added a separate rno-g-extras option now with the runtable, that does not get installed by default. Not sure if there is a better solution; in any case once you try to use the RNO-G reader it suggests installing the run table, I think.

…mprove-optional-deps

sjoerd-bouma · 2025-01-15T12:39:08Z

@fschlueter let me know if you're happy with it now. I also fixed the missing tables in the current documentation - not sure why or when the previous way of writing table headers was deprecated (couldn't find it in the Sphinx changelog) but I just changed it to how https://sublime-and-sphinx-guide.readthedocs.io/en/latest/tables.html suggests to write tables (well, more or less...)

fschlueter · 2025-01-20T09:19:57Z

documentation/source/NuRadioReco/pages/times.rst

+Times in `Channel <NuRadioReco.framework.channel.Channel>`, `ElectricField <NuRadioReco.framework.electric_field.ElectricField>` and
+`Trigger <NuRadioReco.framework.trigger.Trigger>` objects are all defined relative to the
+`station_time <NuRadioReco.framework.station.Station.get_station_time>` of the Station they are stored in (see the description
+of the :doc:`NuRadio data structure </NuRadioReco/pages/event_structure>`). These times are stored as an array of floats.


These times are stored as an array of floats.

To what does this refer?

fschlueter · 2025-01-20T09:23:47Z

documentation/source/NuRadioReco/pages/times.rst

+The global time at which the event takes place is stored as the `event time <NuRadioReco.framework.event.Event.get_event_time>`
+in the `Event <NuRadioReco.framework.event.Event>` object.
+This time usually corrsponds to the "vertex time" of the first interaction for simulations,


For data the event and station time are typically the same, right? The only exception I can think of is that we - as part of an event reconstruction - merge events across stations. Can we add a statement here - just to be very explicit.

fschlueter · 2025-01-20T09:25:15Z

@sjoerd-bouma great work, thank you so much! I have a few more minor comments (I think we should make the documentation explicit AF to never touch it again (or at least not for a very long time).

fschlueter · 2025-01-20T09:33:20Z

NuRadioReco/detector/RNO_G/db_mongo_read.py

+        """
+        returns the current primary measurement of the component, reads in the component collection


Minor comment, I think with the PR #788 I am adding a wrapper around this function within the detector class which probably should be used by users.

Then maybe you can update this docstring / the module docstring in #788? Feel free to copy and paste stuff from here if you think it applies.

fschlueter · 2025-01-20T09:34:33Z

NuRadioReco/framework/trigger.py

+        For some triggers, in addition to the time of the first trigger,
+        also all subsequent times where the trigger condition were fulfilled are


Should we not duplicate a remark there that the trigger time is relative to the station_time?

fschlueter · 2025-01-20T09:40:14Z

documentation/source/NuRadioReco/pages/times.rst


-* hardwareResponseIncorporator (sim to data):
    * the channel traces are folded with the amplifier response which also includes some time delay


I would add a statement that this time delay is reflected by moving the pulse within the trace and not by adjusting the trace_start_time

documentation/source/NuRadioReco/pages/times.rst

sjoerd-bouma · 2025-01-21T14:09:02Z

@fschlueter hopefully I've addressed your remaining comments. I also reformatted a couple of docstrings that were causing warnings during compilation.

sjoerd-bouma added 10 commits September 11, 2024 12:25

add instructions to install development version with pip

66942b8

improve installation of optional dependencies

89e0105

improve and update some installation documentation

1ab79e5

update documentation of optional dependencies

3c54d3a

Merge branch 'develop' of github.com:nu-radio/NuRadioMC into poetry/i…

5293ae7

…mprove-optional-deps

add missing pre_trigger_times to trigger subclass docstrings

e0d5e61

reduce toctree maxdepth for API documentation

a9b2787

add some more documentation for db_mongo_read

bada1f9

tidy up times.rst somewhat

78af870

remove unused debug argument

2dba3b8

fschlueter reviewed Dec 2, 2024

View reviewed changes

documentation/source/NuRadioReco/pages/times.rst Outdated Show resolved Hide resolved

sjoerd-bouma mentioned this pull request Dec 10, 2024

Clean up imports in voltageToAnalyticEfieldConverterNeutrinos #778

Merged

sjoerd-bouma added 2 commits January 10, 2025 14:16

Merge branch 'develop' of github.com:nu-radio/NuRadioMC into poetry/i…

be43926

…mprove-optional-deps

improve documentation of trigger times

b703b62

fschlueter reviewed Jan 13, 2025

View reviewed changes

NuRadioReco/framework/trigger.py Outdated Show resolved Hide resolved

fschlueter reviewed Jan 13, 2025

View reviewed changes

documentation/source/NuRadioReco/pages/times.rst Outdated Show resolved Hide resolved

fschlueter reviewed Jan 13, 2025

View reviewed changes

documentation/source/NuRadioReco/pages/times.rst Outdated Show resolved Hide resolved

fschlueter reviewed Jan 13, 2025

View reviewed changes

documentation/source/NuRadioReco/pages/times.rst Show resolved Hide resolved

fschlueter reviewed Jan 14, 2025

View reviewed changes

sjoerd-bouma added 4 commits January 15, 2025 12:52

further clean up of times documentation

d24a6af

fix documentation table header formatting

2c7d8d8

add option to install all optional dependencies via pip

822bad8

Merge branch 'develop' of github.com:nu-radio/NuRadioMC into poetry/i…

d57e9ae

…mprove-optional-deps

sjoerd-bouma mentioned this pull request Jan 17, 2025

[bugfix] actually adjust trace_start_time in channelStopFilter #793

Merged

fschlueter reviewed Jan 20, 2025

View reviewed changes

documentation/source/NuRadioReco/pages/times.rst Show resolved Hide resolved

fschlueter mentioned this pull request Jan 20, 2025

Fix trigger time and readout delays set by the readRNOGDataMattak module #796

Merged

This was referenced Jan 21, 2025

Use correct docstring format #800

Merged

Simulate trigger channels for RNO-G (simplified) #788

Merged

sjoerd-bouma added 2 commits January 21, 2025 12:59

[doc] minor changes to times.rst

ddec519

[doc] minor docstring fixes

f05ed18

fschlueter approved these changes Jan 21, 2025

View reviewed changes

sjoerd-bouma merged commit 2cfc1a2 into develop Jan 22, 2025
5 checks passed

sjoerd-bouma deleted the poetry/improve-optional-deps branch January 22, 2025 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation / poetry improvements and fixes #773

Documentation / poetry improvements and fixes #773

sjoerd-bouma commented Nov 29, 2024

sjoerd-bouma commented Jan 13, 2025

fschlueter commented Jan 13, 2025

fschlueter Jan 13, 2025

sjoerd-bouma Jan 13, 2025

fschlueter Jan 13, 2025

sjoerd-bouma Jan 15, 2025

sjoerd-bouma commented Jan 13, 2025

fschlueter commented Jan 13, 2025

fschlueter Jan 14, 2025

sjoerd-bouma Jan 14, 2025

sjoerd-bouma Jan 15, 2025

sjoerd-bouma commented Jan 15, 2025

fschlueter Jan 20, 2025

fschlueter Jan 20, 2025

fschlueter commented Jan 20, 2025

fschlueter Jan 20, 2025

sjoerd-bouma Jan 21, 2025

fschlueter Jan 20, 2025

fschlueter Jan 20, 2025

sjoerd-bouma commented Jan 21, 2025

		"""
		returns the current primary measurement of the component, reads in the component collection

		For some triggers, in addition to the time of the first trigger,
		also all subsequent times where the trigger condition were fulfilled are


		* hardwareResponseIncorporator (sim to data):
		* the channel traces are folded with the amplifier response which also includes some time delay

Documentation / poetry improvements and fixes #773

Documentation / poetry improvements and fixes #773

Conversation

sjoerd-bouma commented Nov 29, 2024

sjoerd-bouma commented Jan 13, 2025

fschlueter commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoerd-bouma commented Jan 13, 2025

fschlueter commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoerd-bouma commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fschlueter commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoerd-bouma commented Jan 21, 2025