IAP GITLAB

Skip to content
Snippets Groups Projects

Initial refractive index interface and constant refractive index model.

Merged Remy Prechelt requested to merge rprechelt-refractive-index into master

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 a GetRefractiveIndex(Point const&) -> quantity<dimensionless_d>. I had this return a quantity<> instance to be consistent with all of the other environment interfaces that are unit-full.
  • Since phys/units does not provide any literals for quantity<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 a quantity<dimenionless_d> value). I can change this to dimenionless 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 in testEnvironment.cc.

An exponential refractive index model will be provided in a future MR.

Edited by Remy Prechelt

Merge request reports

Pipeline #1481 passed

Pipeline passed for 32d03c35 on rprechelt-refractive-index

Test coverage 91.50% (-0.10%) from 1 job

Merged by Ralf UlrichRalf Ulrich 4 years ago (Jul 2, 2020 11:07pm UTC)

Loading

Pipeline #1485 passed

Pipeline passed for 67816a15 on master

Test coverage 91.50% (-0.10%) from 1 job

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • changed milestone to %Radio Emission

  • added Radio label

  • Remy Prechelt changed the description

    changed the description

  • Remy Prechelt added 1 commit

    added 1 commit

    • 12a0be63 - Add test for SetRefractiveIndex

    Compare with previous version

  • Remy Prechelt added 1 commit

    added 1 commit

    • a61a703b - Add additional density checks for homogeneous medium.

    Compare with previous version

  • Author Maintainer

    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 Prechelt
  • The 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
    • 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 Ulrich
  • This 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.
  • Author Maintainer

    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 Prechelt
  • I disagree concerning a type for dimensionless quantities. They are by definition bare numbers and need no special type. And literals for bare numbers are just awkward.

  • Author Maintainer

    I'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 Prechelt
  • Remy, is the literal actually needed, or would a double just be fine as suggested by Max? Maybe it really isn't needed and also does not increase the code readability.

  • Author Maintainer

    I'm 100% OK with a raw double (in fact, my first version just used a raw double). I'll update this MR to use a raw double.

  • Remy Prechelt added 1 commit

    added 1 commit

    • 6b024747 - Change interface to use raw double for rindex.

    Compare with previous version

  • Remy Prechelt added 9 commits

    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.

    Compare with previous version

  • Note: the slight drop in coverage is an artefact of our templated environment interface. We have to develop a smart way to test this.

  • merged

  • Ralf Ulrich mentioned in commit 67816a15

    mentioned in commit 67816a15

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading