Initial refractive index interface and constant refractive index model.
Excuse the MR spam but I'm trying to tick off all the boxes needed for me to start testing my radio emission model #276 (closed) . I'd appreciate any and all feedback on the architecture or implementation.
This MR implements:
- A refractive index interface,
IRefractiveIndexModel
that provides aGetRefractiveIndex(Point const&) -> quantity<dimensionless_d>
. I had this return aquantity<>
instance to be consistent with all of the other environment interfaces that are unit-full. - Since
phys/units
does not provide any literals forquantity<dimensionless_d>
and it is not implicitly constructible from a raw numerical type, the only constructor that I'm aware of is (quantity<corsika::units::si::dimensionless_d> myvalue(phys::units::detail::magnitude_tag, 1.000327)
(please let me know if you know a better way). To make this less verbose, I defined a new literal for dimensionsless values of__
(i.e.3__
is aquantity<dimenionless_d>
value). I can change this todimenionless
or something more explicit if people prefer (i.e.3_dimensionless
) but that seems excessivey long. Let me know if you have a better suggestion/idea! - I also implemented a simple instance,
UniformRefractiveIndex
, that provides a constant refractive index everywhere in the environment. This will be useful for a test case for air showers and is physically useful for in-ice showers. - Several unit tests for the
UniformRefractiveIndex
are provided intestEnvironment.cc
.
An exponential refractive index model will be provided in a future MR.
Merge request reports
Activity
changed milestone to %Radio Emission
added Radio label
added Ready for Code Review label
added 1 commit
- a61a703b - Add additional density checks for homogeneous medium.
I've updated this MR with additional checks of the underlying atmospheric density model to fix the low code coverage.
However, this MR also suffers from the extraneous D0/D2 destructor issues discussed in !211 (merged) (unused destructors are included in the binary and instrumented by
lcov
). I have confirmed that under-O2
these destructors are removed from the test binary and the coverage returns to 91.7%.Edited by Remy PrecheltThe code approval procedure is described in the wiki: Code approval procedure wiki
- The MR is without WIP (work in progress) status
-
Make sure the most recent CI jobs (config, quality, build, test, example) all run fine with no failures
- if "check clang-format" failed: the code contributor has to run
./do-clang-format.py --apply
eventually with the--all
option - if "check copyright" failed the code contributor has to run
./do-copyright.py --add=20xy
- if "check clang-format" failed: the code contributor has to run
- Also run all the extra optional jobs "coverage", "release-clang-8", "release-u-18.04" and make sure no problems occur
- Check in the "coverage" job output that the coverage did not decreases (!). It should always stay, or increase. If it decreased --> ask contributor to add further unit tests, and check coverage report.
- On the MR page, open the "Open in Web IDE" tool
- Check if the provided solution solves the Issue, discuss on gitlab
- Check that all changes are actually related to the issue
- There are no debug statements left, not even commented out
- Check all changes for coding rules and guidelines
- When all above is done
- Add label "Code Review Finished"
Edited by Ralf UlrichThis looks very good. The coverage is OK. We understand where this comes from and need to find a solution outside of this MR here.
About the literal for dimensionless:
- this is something very useful, we need it.
- I don't like the '' since it is not expressive at all, in contrary, '' may be used for all kind of things and the meaning is entirely unclear from reading code.
- in plots you often see '1' as unit, but "5_1" is also very strange and misleading.
- thus, I think it has to be something longer but more clear. Either "_dimensionless" or maybe just "_nounit" but while the latter is shorter, the former is still more clear.
I agree about
__
- I was not happy with it either but couldn't think of a better solution at the time. I'm happy with either_dimensionless
or_nounit
. I probably (slightly) prefer_nounit
since (for me) its very clear what it means (a unit-less quantity) but I will defer to you.I'll update the MR with whatever we decide.
I'm working on a solution to the coverage issue.
Edited by Remy PrecheltI'm also 100% OK with using bare numbers for this interface. The only advantage I saw was in being consistent with most other environment-related values/methods using
quantity<>
but that's only a mild advantage (and comes at the cost of some complexity regarding literals).Let me know if there's a consensus on switching this MR to using bare numbers instead of
quantity<dimensionless_d>
Edited by Remy Precheltadded 1 commit
- 6b024747 - Change interface to use raw double for rindex.
added 9 commits
-
6b024747...2a33b1b2 - 6 commits from branch
master
- d2d5dfb4 - Initial refractive index interface and models with unittests.
- 2dc06e42 - Add additional density checks for homogeneous medium.
- 32d03c35 - Change interface to use raw double for rindex.
Toggle commit list-
6b024747...2a33b1b2 - 6 commits from branch
added Code Review Finished label
mentioned in commit 67816a15